units
units copied to clipboard
nholthaus
SFINAE friendly signatures
Take a look at the following code:
using namespace units::literals;
static_assert(!std::is_convertible_v<decltype(0_m), decltype(0_m * 0_m)>); // [1]
units::length::meter_t m{0_m * 0_m}; // [2]
[1] triggers the assert although [2] is an error. That's because the signatures that are used to check the type trait in [1] are valid while the error in [2] is caused by an static_assert in the body of the convert function.
The fix to get this to work would be to move any late checking done in convert to an early check in the signatures of the functions that directly or indirectly call it.
The lack of SFINAE friendliness in the library is causing the concepts I linked to earlier not to work as expected.
You're on the v3.x branch right? Just so you know, that branch is (intentionally) very unstable right now, and has a few planned refactors (including unit type names) that may break code depending on it pre-release. That said, I really appreciate the help you've been providing!
I can't believe I haven't run into this before... except I guess I always rely on the implicit conversions/errors and don't typically concept-check units because of that.
convert should be the last-resort in terms of asserting for incompatible units, but it's not in many places, for reasons, mostly bad ones. There are equivalent traits in the units::traits namespace which is what all the testing is currently done against, but I agree with you that that isn't an acceptable answer. I'll add some testing against std::type_traits, and I can prioritize your favorites if you know which ones your concepts rely on.
It's kind of bad, but although the docs advocate the literals for most use cases, until about 3 weeks ago the library supported vs2013 which had a really limited and buggy c++14 implementation and didn't support literals and variadic templates, so most testing is done against the convert function rather than the unit types and literals themselves. That's part of the big refactor that needs to be done for 3.0, and you may find more weirdness until that transition is done.
Thanks for the warning. I'm well aware of it.
... That's part of the big refactor that needs to be done for 3.0 ...
Hopefully that means that during the refactor the checks done by convert will be moved by to the signatures that use it. That'd be the most general and clean solution, wouldn't require playing favorites which would just add baggage to this library's code, nor require general solutions, like the concepts, to know about units::traits.
... I can prioritize your favorites ...
Thanks for the offer, but the problem is non-blocking.
When implementing this change, you should consider changing the primary templates of unit type traits to not define the type aliases to void, and do like the standard type traits and define no type aliases.
To implement this, what before might have been a compile error with some nice static assertion messages, would become a "novel" of errors, as said in the video linked in the article How to Make SFINAE Pretty – Part 1: What SFINAE Brings to Code from FluentC++. Said video (and presumably the follow-up of the linked article) show how we might mitigate this. I'm already using that technique in #126, which consists on factoring uses of std::enable_if_t with more appropriate names. Another use might be to name the boolean expressions, as modern compiler give special treatment to the first argument of std::enable_if_t to make errors messages clearer.
Even then, the units, being type aliases rather than actual types, will continue to be spelled out in full in the error messages. It has been suggested before to make them different types.
#150 or one of the other PRs made us a lot more sfinae friendly right? If not I don't think we'll do this for 3.0.0
Not really. It only slightly improved existing use of SFINAE. Many signatures still need them, like the in-class unit ordering operators, which should check for is_same_dimension. But it can certainly wait for 3.1.0.