Compare underlying values of hetereogeneous types #432
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Formerly, in `Quantity`, we used a `CompareUnderlyingValues` helper, and a `UnitSign` enum. The helper was responsible for taking two _identical_ `Quantity` types, and extracting their values. (We built out this machinery to support negative units.) Now, we refactor to use a new helper, `SignAwareComparison`. This brings two simplifications and one generalization. - **Simplification:** stop doing the extraction in the helper; let the callers extract and pass the values. (This gives the helper just one responsibility: namely, take the unit sign into account when deciding which order to pass the parameters.) - **Simplification:** lose the enum; just use the new `UnitSign` trait. - **Generalization:** accept _heterogeneous_ parameters. This last change will allow us to detect when we are passing mixed signed and unsigned integral arguments. Of course, this PR is a no-op refactor, so we're not actually taking advantage of this yet. That will come in a follow-on PR.
hoffbrinkle
reviewed
May 20, 2025
Co-authored-by: Michael Hordijk <hordijk@aurora.tech>
hoffbrinkle
approved these changes
May 21, 2025
This was referenced Aug 19, 2025
chiphogg
added a commit
that referenced
this pull request
Aug 19, 2025
We did not mean to disable these in #432. Maybe my excuse is that my brain was already living in the glorious future where #122 is landed. At any rate, thank goodness for those policy argument overloads! This would have been a much more complicated fix without them. I tested this by uncommenting the "feeters" test case, and verifying that it now produces a compiler error, as it should. I also uncommented the quantity point test case where we check inheriting the overflow safety surface. Fixes #516.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Formerly, in
Quantity, we used aCompareUnderlyingValueshelper,and a
UnitSignenum. The helper was responsible for taking twoidentical
Quantitytypes, and extracting their values. (We builtout this machinery to support negative units.)
Now, we refactor to use a new helper,
SignAwareComparison. Thisbrings two simplifications and one generalization.
Simplification: stop doing the extraction in the helper; let the
callers extract and pass the values. (This gives the helper just one
responsibility: namely, take the unit sign into account when deciding
which order to pass the parameters.)
Simplification: lose the enum; just use the new
UnitSigntrait.Generalization: accept heterogeneous parameters.
This last change will allow us to detect when we are passing mixed
signed and unsigned integral arguments. Of course, this PR is a no-op
refactor, so we're not actually taking advantage of this yet. That will
come in a follow-on PR.
Helps #428.