Show Posts
1
SFML development / Sensible use of certain C++17 features
« on: December 08, 2021, 11:06:29 am »
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:
Same for methods which could be interpreted in a different way due to English language ambiguity:
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?
constexpr
See also this comment.
Similarly, we could apply constexpr to a ton of APIs. But it comes again at a cost:
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
SFML currently uses (a) for implicit conversions or single-value initializations and (b) for constructor calls:
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:
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:
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.
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!
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!
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
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)
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);
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
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;
// 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.
