-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #580 by using a fixed-point implementation for unit conversions using integer representations #615
base: master
Are you sure you want to change the base?
Conversation
Th hi; | ||
Tl lo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make those private
? In case we do, can we name them with the _
postfix to be consistent with all other places in the library?
}; | ||
|
||
|
||
constexpr double_width_int operator+(Tl rhs) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be a non-member two-parameter function to make sure that conversions work for lhs and rhs? Should there be another overload with reversed arguments?
Tl lo; | ||
}; | ||
|
||
#if false && defined(__SIZEOF_INT128__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, CI complains about the true-path because __int128_t
is non-standard. Given that this is an implementation-detail, I should probably look for a way to disable that warning instead.
#endif | ||
|
||
template<typename T> | ||
inline constexpr std::size_t integer_rep_width_v = std::numeric_limits<std::make_unsigned_t<T>>::digits; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inline
not needed from variable templates.
|
||
template<std::integral U> | ||
requires(integer_rep_width_v<U> <= integer_rep_width_v<T>) | ||
auto scale(U v) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constexpr
?
fractional_bits); | ||
} | ||
|
||
repr_t int_repr_is_an_implementation_detail_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private
?
/* using multiplier_type = conditional< | ||
treat_as_floating_point<c_rep_type>, | ||
// ensure that the multiplier is also floating-point | ||
conditional<std::is_arithmetic_v<value_type_t<c_rep_type>>, | ||
// reuse user's type if possible | ||
std::common_type_t<c_mag_type, value_type_t<c_rep_type>>, std::common_type_t<c_mag_type, double>>, | ||
c_mag_type>;*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why to comment this code instead of removing it?
Most of the review concerns should be fixed now. Also, I "fixed" the CI failure in the trigonometry tests by allowing the affected test-cases to deviate by two instead of one epsilon. I believe this appears because now, for What I did not yet address are the discussions about making the implementation details |
That CI failure for GCC 12 and GCC 13 |
I was not aware of this. This makes sense... But in such a case, I would obfuscate the names much more (similar to what we have in |
I think you did not check if properly. GCC complains about |
test/static/CMakeLists.txt
Outdated
@@ -36,6 +36,7 @@ add_library( | |||
custom_rep_test_min_impl.cpp | |||
dimension_test.cpp | |||
dimension_symbol_test.cpp | |||
fixed_point.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please postfix the file name with _test
You are right of course. It just happens that |
I should probably increase the test coverage for that |
This PR provides an implementation of what I played with in this comment to issue 580. Basically, it completely distinguishes floating-point and integral paths. For floating-point paths, the conversion implementation remains the same (a single multiplication with a floating-point representation of the conversion factor). For the integral paths, it now also uses a single multiplication, but now with a fixed-point representation. (mixed conversions take the floating-point path). Fixed-point multiplications have the advantage that they are comparatively cheap (much cheaper than a division) and can accurately describe all reasonable conversion factors between two n-bit numbers using a n.n (2n total) fixed-point representation. Unreasonable conversion factors are those larger than 2^n or smaller than 2^-n, i.e. those which either overflow for all input values or underflow for all input values. The cost of such a fixed-point multiplication is at most that of 4x n-bit integer multiplication plus some bookkeeping if their output is restricted to n-bit (as in the C++ standard; unlike typical hardware); if the output of a n-bit multiplication can be 2n (most instruction-sets provide those), it will just take two of them.
With the implementation change here, the computation of conversions between two quantity types of the same dimension and both integer type stay tightly within the types mandated by the source and destination type, without expanding to intmax unless necessary. In general, this will never cause overflows unless the result type actually cannot carry the result (the fixed-point multiplication is guaranteed to be sufficient). However, this practice triggered the weakly related #614, where it turned a silent overflow into a compilation failure. For now, this PR just disables those two test. Once we have a fix for that one, we should re-enable those tests and rebase this PR.