What other types may need to get this treatment? The pattern is that types that hold a raw pointer to a resource ought to not have a default constructor.

sf::Sound is a potential candidate for this change. The problem with sf::Sound is that when copied that pointer to the sound buffer goes null so to do this we'd have to change the copy semantics of sf::Sound. Maybe we remove copy semantics in favor of making it a move-only type to address this.

Codecov Report

Merging #2494 (25f1d0c) into master (839ad6c) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2494      +/-   ##
==========================================
- Coverage   27.28%   27.28%   -0.01%     
==========================================
  Files         228      228              
  Lines       19639    19637       -2     
  Branches     4710     4709       -1     
==========================================
- Hits         5358     5357       -1     
- Misses      13802    13821      +19     
+ Partials      479      459      -20     

What other types may need to get this treatment? The pattern is that types that hold a raw pointer to a resource ought to not have a default constructor.

Possibly some of shapes?

CircleShape (float radius=0, std::size_t pointCount=30)
RectangleShape (const Vector2f &size=Vector2f(0, 0))

Are these meaningful defaults? Not necessarily.

But it's important that we're all fully aware that we do restrict quite some use cases by not having things default-constructible, like:

• std::array / raw arrays
• std::vector::resize
• std::map::operator[]
• conditional initialization with if/else
• ...

sf::Sound is a potential candidate for this change. The problem with sf::Sound is that when copied that pointer to the sound buffer goes null so to do this we'd have to change the copy semantics of sf::Sound.

It doesn't seem to reset to null?

This auto-attach/detaqch seems to me more like something where we could unify the behavior of resources. It can be surprising that sound auto-unlikes itself upon destruction, while others just run into UB. I understand it's probably because sounds run continuously on a separate thread and sounds need to be centrally managed, but still...

Maybe we remove copy semantics in favor of making it a move-only type to address this.

That seems too restrictive to me. These "frontend" classes (sprite, sound, text) are relatively lightweight and I don't see why we should forbid copying.

It doesn't seem to reset to null?

What other types may need to get this treatment? The pattern is that types that hold a raw pointer to a resource ought to not have a default constructor.
Possibly some of shapes?

CircleShape (float radius=0, std::size_t pointCount=30)
RectangleShape (const Vector2f &size=Vector2f(0, 0))

Are these meaningful defaults? Not necessarily.

If a type can only reach a given state through its default constructor, that default constructor maybe shouldn't exist. For example, a std::vector is default constructed with zero elements, but it's still valid to call .clear() to get back to that state later. Seems reasonable to me.

However, an sf::Sprite can only have a null texture upon default construction and not through any calls to setTexture. The fact that setTexture takes a const reference implies that we don't want users to nullify that texture later. It implies to me that having a null texture was never an intentionally valid state but rather a quirk of development that was never second-guessed. This argument probably applies to sf::Sound but apparently details of OpenAL complicate things so more research will be required.

By that logic I think it's fine to let an sf::CircleShape or sf::RectangleShape default construct with a zero size because that's a state you can already achieve in the API. We wrote an API that says "zero size is valid" so it's also valid to have a default constructor that takes advantage of that possibility.

TL;DR

• Forcing users to specify some initial elements would be annoying when an empty std::vector is already a permissible state to put a vector into.
• Forcing users to specify a texture when constructing an sf::Sprite makes sense because there is no way to nullify that texture later.
• Forcing users to specify a radius/size on a sf::RectangleShape or sf::CircleShape would be annoying when it's already valid to set those values to all zeros.

If a type can only reach a given state through its default constructor, that default constructor maybe shouldn't exist. For example, a std::vector is default constructed with zero elements, but it's still valid to call .clear() to get back to that state later. Seems reasonable to me.

However, an sf::Sprite can only have a null texture upon default construction and not through any calls to setTexture. The fact that setTexture takes a const reference implies that we don't want users to nullify that texture later.

This is a great observation, makes total sense 👍

I'm not sure if it alone is a sufficient criterion though. std::vector being empty by default is not only reasonable because one can clear it, but also because it's a very useful default. In a ton of scenarios, one adds only elements over time (like bullet entities in a game), and starting out empty is desired.

In contrast, a rectangle or circle with 0 extents is a degenerate case and almost never what someone wants -- even in the few hard-to-construct scenarios where someone intentionally needs this, it's always possible to explicitly provide such arguments.

To be clear, I'm not saying sf::Shape derivates cannot have default constructors. But these default constructors help mainly to get around technical limitations (e.g. using std::array), but are semantically useless.

In contrast, a rectangle or circle with 0 extents is a degenerate case and almost never what someone wants -- even in the few hard-to-construct scenarios where someone intentionally needs this, it's always possible to explicitly provide such arguments.

I think it would be reasonable to force an explicit radius in many cases (but not in all of them; as a member variable it would be annoying), but I'd also consider that even with the defaulted radius of 0 the object in itself is still valid. The other calls are not failing after all.

Also, using this approach, one could even question if it should be possible to default a (geometrical) vector to all zeroes, because a zero length vector would be a special edge case just like a zero radius circle.

I think it would be reasonable to force an explicit radius in many cases (but not in all of them; as a member variable it would be annoying)

Is "annoying" a good enough reason though? It's also annoying to be forced to initialized sf::Text in the constructor; there may not be a sf::Font available at the time. On the other hand, it gives you the guarantee that whenever you see a sf::RectangleShape object, it is already initialized.

Also, using this approach, one could even question if it should be possible to default a (geometrical) vector to all zeroes, because a zero length vector would be a special edge case just like a zero radius circle.

Fully disagree, zero vectors are geometrically meaningful for a huge number of cases: coordinate origin, zero offset, zero velocity, etc. They are a very reasonable default value, the same way that 0 is for int.

But just like a zero velocity make sense (even if there's no direction), wouldn't a zero radius circle make sense? A vector is defined by length and orientation, yet a zero vector lacks a clear orientation. A circle is defined by a radius (and number of segments, technically), so would a zero radius circle still be a circle? Maybe getting a bit too philosophical…😉

I think sf::Sprite is a completely different case. It's a visual representation of a part of a texture, so without a texture it doesn't make any sense, unless you want to do drawing in shaders only. So maybe even introduce a sf::Texture::None static for cases where the texture isn't existing?

It doesn't seem to reset to null?

SFML/src/SFML/Audio/Sound.cpp

Line 172 in 21af6fe

m_buffer = nullptr;

This is removing the current buffer from the sound to which the new sound has been applied so that the new sound's buffer can replace it (since we're copying from that sound):