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.
constexprSee 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.
noexceptPrimarily useful on move constructors, swap, destructors.
Benefit outside such special methods is questionable in SFML.
explicitUseful for constructors that can act as conversion constructors (callable with 1 argument) or conversion operators.
Uniform initializationint 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.
EmplacementGood 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 typesIn 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.
Show Posts
