Open JohelEGP opened 5 years ago
very clever issue name my friend. You're right, and I think that our 4 or 5 time replace all on the name unit
has led us to the right name for the trait... how do you feel about is_same_dimension
?
The second paragraph I need to prototype. IIRC the reason for defaulting to void was that compilation would fail when using the dimension traits if the two types weren't of the same dimension, but hopefully we've moved past that now.
how do you feel about
is_same_dimension
?
Sounds great, and conforming to the naming of the standard traits, I believe.
but hopefully we've moved past that now.
Maybe. I once tried to remove them and met compiler errors. Once everything's properly constrained, we should be able to remove them.
There is also a lying template name, if I may :)
NonLinearScale optional scale class for the units. Defaults to linear
I think it's confusing to have a NonLinear concept encompass a linear variety.
Calling it something like 'NumericalScale' would be clearer.
An example of the void
issue is detail::is_convertible_conversion_factor
, which adds logic to is_convertible_conversion_factor
. It needs additional checks in case the trait's result would lie so that it can be successfully used in SFINAE contexts.
NonLinearScale
renaming should be accompanied with a is_nonlinear_scale
renaming, too. The caps on it suggest that a _
is missing between non
and linear
.
I think is_nonlinear_scale
is a lying trait, too, based on how unit
uses it. It only tests for default constructability, but the tested type is constructed from the underlying type.
I think the classes that implement this concept can be simplified and improved by dropping the Args
variadic template argument of their constructor and instead using a store_linear_value
tag. linear_scale
can default it and decibel_scale
overload based on it. Then we can rid ourselves of those std::true_type() /*store linear value*/
lines and use store_linear_value()
.
is_convertible_unit
also lies. It defers to is_convertible_conversion_factor
, but now that we check for lossy conversions, it's no longer true.
is_convertible_unit
also lies. It defers tois_convertible_conversion_factor
, but now that we check for lossy conversions, it's no longer true.
Perhaps it should also be renamed to is_same_dimension
, and let std::is_convertible
be used for the latter. But then the SFINAE conditions that use is_convertible_unit
would also have to check its arguments with is_unit
so that they don't participate in overload resolution when both happen to be conversion_factor
s.
Another case of lying traits are the is_
dimension_unit
traits when used with conversion_factor
s. The _unit
suffix is the lying part.
I think that everything but the comment above is resolved.
is_convertible_conversion_factor
is not about testing whether aconversion_factor
is convertible to another, but whether their dimensions are the same, as suggested by its implementation. Maybe it's its name that needs to be improved.The traits with a
type
member alias shouldn't default tovoid
, but to having notype
member, likestd::enable_if
. Remember thatstatic_assert
that never failed because it eventually indirectly evaluatedstd::is_same_v<void, void>
?