Sensible use of certain C++17 features

[Nexus]

While doing the C++17 migration, it's probably good if we settle down on some conventions regarding the use of newer C++ features. A lot of features are intended for a specific purpose and highly useful within that scope, but not intended as a broadband catch-all measure to be thoughtlessly applied to all code.

I'd suggest to be pragmatic during the migration, which means: we avoid unnecessary changes to existing code where benefits are mostly theoretical, and we focus on C++17 features which directly benefit the user (the currently ongoing PRs are a good example of this! )


A non-exhaustive list + my opinions:


[[nodiscard]]

From this comment:

[[nodiscard]] is primarily useful to prevent mistakes in non-obvious cases where a return value could be accidentally ignored. For example:

sf::Image image;
image.loadFromFile(...);
// WARN: didn't check 'bool' return type!

Same for methods which could be interpreted in a different way due to English language ambiguity:

std::vector<T> container = ...;

container.empty(); // empty the container
// WARN: this is an adjective, not a verb!

In other words: "should the caller use the return value" is a necessary, but not sufficient criterion for  [[nodiscard]].

95% of all functions which have a return value intend that to be used. We don't want that attribute on functions like Sprite::getPosition() or Sound::isRelativeToListener(), even if they'd technically qualify. Why not?

• It makes APIs less readable, making function signatures more complex without adding meaningful information
• The cases where where [[nodiscard]] is justifiably used are no longer recognized as such, thus the attribute loses its intended meaning.
• We should prevent likely errors while not sacrificing ergonomics for unlikely ones.

constexpr

See also this comment.

Similarly, we could apply constexpr to a ton of APIs. But it comes again at a cost:

• more complex API signatures
• the necessity to move implementation to the header (which can in turn require exposing dependencies in headers)
• restricted freedom in implementation (the function can no longer just compute values at runtime, cache them, or similar) -- removing constexpr becomes a breaking change

We should probably focus constexpr on value-like classes which are likely benefitting from being compile-time computable (or at least declarable), such as sf::Color.


noexcept

Primarily useful on move constructors, swap, destructors.
Benefit outside such special methods is questionable in SFML.


explicit

Useful for constructors that can act as conversion constructors (callable with 1 argument) or conversion operators.


Uniform initialization

int value = 3;    // (a)
int value(3);     // (b)
int value{3};     // (c)
int value = {3};  // (d)

SFML currently uses (a) for implicit conversions or single-value initializations and (b) for constructor calls:

int value = 3;
sf::Color a = sf::Color::Red;
sf::Color b(255, 0, 0);

I think we should stay with this convention, this also reduces amount of code to be changed for cosmetic reasons.

What we can do however is:

MyCStruct aggregate = {};
std::vector<int> v = {1, 2, 3}; // std::initializer_list, not uniform initialization

Type inference (auto + decltype)

We should use auto where it helps increase readability. Iterator declarations are such an example, also repetition of type names are potential candidates:

auto red = sf::Color::Red;
// instead of
sf::Color red = sf::Color::Red;

I would not use it where the type information helps the understanding of the code, or in function signatures or return types. It's also possible to introduce bugs by propagating a wrong type through multiple type-inferred functions.

decltype is typically used in generic code when it's hard to deduce the type of an expression. I don't think SFML has many places where it's needed.



Emplacement

Good if heavy copies can be avoided (moving SFML resource classes into a container). Otherwise, similar thoughts as in type inference section apply: it can make some code harder to understand, while not making a notable difference (due to optimization of temporary objects, or simply small enough sizes). Depends a bit on the context, good variable names can also mitigate that.

We should particularly avoid emplace_back(), when push_back() does the job (that will just cause an extra copy constructor call).


Trailing return types

In my opinion only useful when the return type depends on the parameter types. As such, we should limit their use to generic code which truly needs them.

[reconn]

Well described! I do support and agree with most of what you said. Sensible use but taking advantage when it matters.

I would add that other uses for noexcept come when an exception should not happen because it will never be caught anyways. For example, "threaded functions" like in void SoundStream::streamData().

Regarding auto/type deduction. C++17 also added CTAD which helps with readability when both auto and explicit type are clunky. So as an advice, if auto seems to be a good idea, maybe check if CTAD isn't better. Example:

// here: it's better to use auto
auto renderTextureImpl = std::make_unique<RenderTextureImplDefault>();
// and now transfer ownership
auto otherImpl = std::move(renderTextureImpl);  // less obvious that it's a pointer
std::unique_ptr<RenderTextureImplDefault> otherImpl = std::move(renderTextureImpl);  // too verbose type
std::unique_ptr otherImpl = std::move(renderTextureImpl);  // somewhere in the middle
 

or

std::scoped_lock lock(m_mutex);
// vs.
std::scoped_lock<std::mutex> lock(m_mutex);