Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
Fix #580 by using a fixed-point implementation for unit conversions using integer representations #615
Changes from 6 commits
cf9c05f
3635260
74f30da
2f54c76
e003a58
60c94da
d00b330
99d4315
653d3d2
ed2574f
e688ffc
55d8fd6
38dcf64
ad76149
95cc9f3
5f8eb5c
1b57404
f673619
f642d37
b6a6752
647ce6b
464ecd4
e933be7
6873c8b
65a0ee4
0c1971e
4ef0210
35ed472
329b9f5
7fa15d2
e464677
cc9ea9d
a51462c
f4c8e90
01f44c6
5713243
ff11878
b35e241
ef0e7b3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If I am not wrong, this class only works at compile-time. If that is the case, then all its interfaces should be
consteval
. If some older compilers will complain, then we have anMP_UNITS_CONSTEVAL
workaround.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.
Some of the arithmetic operators are needed at runtime for the fixed-point scaling (scaling an
i64
with a constexpr fixed-pointi64.64
is implemented as a multiplication by ani128
followed by a right-shift by 64 bit). There is a bit of freedom in what rounding behaviour we'd like to have, which would require support for some runtime addition/subtraction as-well.On the other hand, constructors could be made
consteval
for the fixed-point use-case. That said, I was just increasing test coverage for the arithmetic operators, which I am currently writing as runtime tests. The goal is to test a significant number of value combinations, and while this could be done at compile-time, I fear for the compilation time :-).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.
Is this a factory function? If so, shouldn't the constructors be
private
?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.
This is a factory function, but note that it is not equivalent to the constructor taking a
(hi, lo)
pair.wide_product_of(lhs,rhs)
creates an instance representinglhs * rhs
, whiledouble_width_int(hi,lo)
creates an instance representing(hi<<base_width) + lo
.One could argue though that there is a certain risk of confusion about the semantics of the two-argument constructor, so we may want to still make it private and instead expose a factory function
from_nibbles
or similar.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?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.
I don't have strong feelings here. I was thinking it could be relevant for this type to be a literal type, given that we intend to use it mostly at compile-time. Perhaps it becomes useful in some numeric manipulation needed for magnitudes? Perhaps we want to store an offset between two quantity points/origins with 128 bits of precision? However, I'm aware that literal types are only needed if we want instances as NTTP, and neither of these use-cases definitely require a literal type. I'll make them private, we can still revert that later.
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.
I think you meant structural type? litearal types are types that can be used in
constexpr
functions and those do not need public members.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.
Ah yes, of course.
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.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.
Let's use
consteval
here as well.I understand that in other libraries this might be useful as a runtime-enabled type. However, this is my primary concern here. External libraries might start to depend on our implementation details because of this. This will make us harder to change the bits if needed without breaking user's code. We would be unable to remove it from the repo if we possibly find a better solution, or change its implementation (ABI breaks).
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.
Ok, that is a valid concern. Users should rather use a dedicated fixed-point library such as CNL.
There is still the issue of testing though, I fear about the compilation time if we are going to test a significant number of values and value combinations.
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.
I do not think it will be worse than the thousands of tests we have for quantities already. But if some members are needed at runtime, then it makes sense to keep this type of runtime-enabled.
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
?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.
Again, literal type, but again, I don't actually need that right now. I'll make it private (... and remove the
is_an_implementation_detail_
). We can still change it if a need comes up.