Some last remarks

[Nexus]

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

• 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.
  
  
• 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.
  
  
• VertexArray::VertexArray(PrimitiveType type, unsigned int vertexCount = 0)

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.

• Packet, InputStream, resource classes
  Some work with void* and some with char* for binary data.
  
  
• Window, Image, Texture
  Vector2u getSize() vs. unsigned int getWidth()/getHeight()
  
  
• Vector2f RenderTarget::convertCoords (unsigned int x, unsigned int y) const
  Considering the fact that most functions return vectors (especially Mouse::getPosition() which is often used in combination), it could be wiser to take a sf::Vector2u/i.
  
  

[Laurent]

He he, I was expecting this kind of last minute report

Thanks.

[Nexus]

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)?

[Laurent]

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.

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".

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.

[Fiquet]

Hi everybody,

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?

[Laurent]

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.

[Tank]

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).

[Nexus]

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.

[Laurent]

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.

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).

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.

Even the standard is sometimes inconsistent.

What do you mean?

[Nexus]

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:

• std::count() usually returns std::ptrdiff_t.
  
• std::vector::size() usually returns std::size_t.
  
• std::basic_istream::read() takes std::streamsize, although the source could be a std::vector<char>. (streamsize is signed, but the only place where negative values are used is inside the deprecated std::strstream class).
  

"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