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 3 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.
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?
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.
inline
not needed from variable templates.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 we move unit tests to a dedicated unit test file. We do not want to obfuscate our production with unit tests and pay for them at compile-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.
constexpr
?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.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?
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.
What about
MyInt
? Shouldn't it usefixed_point
as well?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.
What is
MyInt
?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 mean some user-defined wrapper type for an integral type. It will not satisfy
std::integral
but behaves like one. This is why I usedtreat_as_floating_point
.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 right. The problem is that for fixed point types (including integers), we also need to know their range and/or bit width, and their "resolution" (1 for integers). The standard currently lacks a standard way to communicate those for numeric types, so we'd need to introduce further customisation points. Therefore, I currently believe it is better if just provide a general customisation point for scaling by a magnitude. If the custom type really is just a wrapper of an integer, they can just unwrap that integer, feed it back to our customisation point and then wrap the result again.
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.
You can also use
value_type_t
to get and analyze the underlying representation 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.
Ah, I remember having seen that one in the
sudo_cast
implementation. How isvalue_type_t
specified? Its implementation looks likestd::type_identity
, but I guess it is intended to act as a customisation point allowing to declare that a specific typesT
is? / has? / behaves like? / represents a? value of typevalue_type_t
. A quick search however did not turn up any nontrivial use of it however.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.
or
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.
Nope. I don't think either of these suggestions are good choices here, but we may indeed want to refactor here. Right now, this
std::is_integral_v<T>
check really represents thetreat_as_floating_point
behavioural choice here. So thisif
here should really be replaced by a customisation point for custom representation types. I would decidedly not try to "guess" a scaling method here, but rather make it an explicit choice somewhere in the scaling machinery.Instead, I believe I should refactor the
conversion_value_traits
a bit: In fact, these are reallyscaling_traits
in theScalable
concept discussed recently. Right now,conversion_value_traits
are separated by the properties of the scaling magnitude, but I believe this should be reversed: The "outer level" is the potentially customisable scaling behaviour of the representation types. The implementation of that scaling behaviour for different magnitude types is almost an implementation detail and thus should be at the "inner level".