SFML community forums

General => General discussions => Topic started by: Nexus on March 16, 2012, 07:51:04 pm

Title: Some last remarks
Post by: Nexus on March 16, 2012, 07:51:04 pm
Since SFML 2 stands short before its release, I thought I could mention some smaller issues and name inconsistencies as long as they can still be fixed. I don't expect you to apply all suggestions, but maybe there are some in my list you find appropriate ;)


Lock::Lock(Mutex& mutex)
Sound::Sound(const SoundBuffer& buffer)
Ftp::Response::Response(Status code = InvalidResponse, const std::string& message = "")[/b]
You might want to make the constructors explicit. On the other side, I assume the implicitness of sf::RenderStates constructors is on purpose to allow shortcuts.

Title: Some last remarks
Post by: Laurent on March 17, 2012, 11:50:16 am
He he, I was expecting this kind of last minute report :P

Thanks.
Title: Re: Some last remarks
Post by: Nexus on April 03, 2012, 07:24:34 pm
By the way, do you plan to adapt also the constructors and create functions taking two unsigned ints width and height? Or do you keep them for simplicity (with the possibility to add an overload later)?
Title: Re: Some last remarks
Post by: Laurent on April 03, 2012, 08:04:09 pm
No I won't touch them, it would involve too many modifications.

I've finished everything that I wanted to modify, SFML should be ready for the RC.

Quote
Thread::Thread((void(C::*function)(), C *object)
The object parameter may not be NULL, thus it should be a reference, conforming to the general SFML style.
I want it to be clear enough that the pointer will be used later; with a reference I expect people to pass local instances and get undefined behaviours. Moreover, this overload will often be used inside classes, with "this".

Quote
View::reset(const FloatRect& rectangle);
The name suggests that this function has the same semantics as calling the constructor with this signature, however the viewport remains unchanged. setRect() might be an alternative.
This is the complete sf::View class that needs to be redesigned. I can't do it now, it would require too many changes.
Title: Re: Some last remarks
Post by: Fiquet on April 04, 2012, 12:58:31 am
Hi everybody,

Quote from: Laurent on April 03, 2012, 08:04:09 pm
Quote
Thread::Thread((void(C::*function)(), C *object)
The object parameter may not be NULL, thus it should be a reference, conforming to the general SFML style.
I want it to be clear enough that the pointer will be used later; with a reference I expect people to pass local instances and get undefined behaviours. Moreover, this overload will often be used inside classes, with "this".

Just to mention that once I had a "problem" with this design within a project using SFML. VC++ 2010 gave me a warning:

Code: [Select]
warning C4355: 'this' : used in base member initializer list
Because the only way to construct a sf::Thread with the current object is indeed to call its constructor with this in the initiazer list. However the code worked properly.

Perhaps would it be worth to add the possibility of setting this parameter after the object construction itself?
Title: Re: Some last remarks
Post by: Laurent on April 04, 2012, 08:08:31 am
Quote
Perhaps would it be worth to add the possibility of setting this parameter after the object construction itself?
I don't want a sf::Thread object to be able to be half-initialized, in an inconsistent state. I understand this warning, it could be dangerous to use this in the initializer list since the object's members are not fully constructed, but if you know what you do, it is just annoying. You should just disable it.
Title: Re: Some last remarks
Post by: Tank on April 04, 2012, 12:15:06 pm
I'd really not rely on the compiler here. Rather delay the construction with unique_ptr or at least a raw pointer, but do not disable a warning that might show you problems (which may arise later and won't be detected then).
Title: Re: Some last remarks
Post by: Nexus on April 08, 2012, 11:36:05 am
By the way, why do you use std::size_t and unsigned int in the same function for "count" parameters? ;)
bool sf::SoundBuffer::loadFromSamples(const Int16* samples, std::size_t sampleCount,
                                      unsigned int channelCount, unsigned int sampleRate);
Also in other audio functions. Otherwise, you use std::size_t mainly for byte sizes...

[OffTopic] I actually don't like std::size_t at all, it has such an unclear application field. There's size_t, std::size_t, std::ptrdiff_t, unsigned int, ... Even the standard is sometimes inconsistent.
Title: Re: Some last remarks
Post by: Laurent on April 08, 2012, 06:00:30 pm
Quote
By the way, why do you use std::size_t and unsigned int in the same function for "count" parameters?
The first one is std:size_t because it represents the size of an array.
The second one is just an unsigned integer, so there's no need to use a specific typedef.

Quote
I actually don't like std::size_t at all, it has such an unclear application field.
To me it's very clear: as far as I know (can't check the standard right now) std::size_t represents sizes in memory (of objects or arrays).

Quote
There's size_t, std::size_t, std::ptrdiff_t, unsigned int
They all have their own definition, which is very clear. I personnally see no problem with them.

Quote
Even the standard is sometimes inconsistent.
What do you mean?
Title: Re: Some last remarks
Post by: Nexus on April 08, 2012, 09:30:28 pm
Yes, but the terms "sizes in memory", "differences between pointers" and "counts of objects" overlap strongly in their meaning.

For example, some inconsistences in the standard:
"Usually", because the actual return types depend on the iterators/allocators (difference_type and size_type typedefs).

While these loads of typedefs leave freedom for the implementation, the user has to perform (apparent) conversions in many situations, sometimes even explicitly to prevent unjustified compiler warnings. That's why I try to reduce the amount of integral types in my own APIs, but that's just my personal philosophy :)