Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: Some last remarks  (Read 5574 times)

0 Members and 1 Guest are viewing this topic.

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6286
  • Thor Developer
    • View Profile
    • Bromeon
Some last remarks
« 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 ;)

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

Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32504
    • View Profile
    • SFML's website
    • Email
Some last remarks
« Reply #1 on: March 17, 2012, 11:50:16 am »
He he, I was expecting this kind of last minute report :P

Thanks.
Laurent Gomila - SFML developer

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6286
  • Thor Developer
    • View Profile
    • Bromeon
Re: Some last remarks
« Reply #2 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)?
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32504
    • View Profile
    • SFML's website
    • Email
Re: Some last remarks
« Reply #3 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.
Laurent Gomila - SFML developer

Fiquet

  • Newbie
  • *
  • Posts: 7
    • View Profile
Re: Some last remarks
« Reply #4 on: April 04, 2012, 12:58:31 am »
Hi everybody,

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?

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32504
    • View Profile
    • SFML's website
    • Email
Re: Some last remarks
« Reply #5 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.
Laurent Gomila - SFML developer

Tank

  • SFML Team
  • Hero Member
  • *****
  • Posts: 1486
    • View Profile
    • Blog
    • Email
Re: Some last remarks
« Reply #6 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).

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6286
  • Thor Developer
    • View Profile
    • Bromeon
Re: Some last remarks
« Reply #7 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.
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32504
    • View Profile
    • SFML's website
    • Email
Re: Some last remarks
« Reply #8 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?
Laurent Gomila - SFML developer

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6286
  • Thor Developer
    • View Profile
    • Bromeon
Re: Some last remarks
« Reply #9 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:
  • 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 :)
« Last Edit: April 08, 2012, 09:33:11 pm by Nexus »
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

 

anything