Change implicit rep to natural output type#685
Open
chiphogg wants to merge 7 commits into
Open
Conversation
From now on, whenever we do not name the repin a conversion, we'll
obtain whatever type we would naturally get if we wrote out the
conversion by hand. The main places this makes a difference are
promotable integers, and expression templates such as Eigen.
To get the mechanics right, we need to make a bunch of updates to our
`:conversion_strategy` target. Many of these involve explicitly
templating intermediate result types, instead of assuming they're the
same as the input type. But the main new interface property is that _an
output rep of `void` is our signal for omitting the final cast
operation_.
Then, we take advantage of this by changing our implicit-rep conversion
functions in `Quantity` to use `void` as the rep type. (`QuantityPoint`
follows naturally because it uses `Quantity` under the hood.)
We also need to tweak our overflow risk check in `in_impl`. We need to
make the integer promotion carve-out explicit.
One common forced downstream change is that our conversion risk checkers
need to change to their explicit-rep versions in explicit-rep contexts
that we didn't formerly recognize as such. The main place this crops up
is in `Constant`, where we had formerly been manifesting the constant as
`T{1}`, and then just using implicit rep to convert to the target unit.
Finally, as prep for the vector/matrix changes, I realized our ordering
in the main `PermitAsCarveOutForIntegerPromotion` implementation was not
as efficient as it could be. I decided to move the `is_integral` checks
closer to the top, as they should be a lot more common than promotion
and unsigned checks, which don't really even make sense for non-integral
types.
Helps #484. This PR is a major unblocker for Eigen being usable and
performant.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes “implicit rep” conversions (i.e., .in(unit) / .as(unit) without an explicit rep) to return the natural C++ result type you’d get by writing the conversion expression manually (notably impacting integer promotion and expression-template types like Eigen). It accomplishes this by treating an output rep of void as the signal to omit the final cast in the conversion operation pipeline.
Changes:
- Extend
:conversion_strategyso conversions can model intermediate/output types correctly, and interpretNewRep = voidas “no final cast; return natural output type”. - Update
Quantityimplicit-rep conversion paths to usevoidoutput rep and adjust overflow-risk logic to explicitly account for integer-promotion carve-outs. - Update
Constantto run overflow/truncation checking and conversions in explicit-rep mode where appropriate; refresh and add tests to reflect the new semantics.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| fuzz/quantity_runtime_conversion_checkers_test.cc | Updates fuzz test to use explicit-rep lossiness checking where needed. |
| au/quantity.hh | Switches implicit .in()/.as() to void output rep, adjusts overflow carve-out handling, and updates runtime conversion checker helpers. |
| au/quantity_test.cc | Adds/updates tests for integer promotion behavior and distinguishes implicit vs explicit rep overflow checking. |
| au/conversion_strategy.hh | Implements NewRep = void behavior and corrects intermediate typing for rational conversions. |
| au/conversion_policy.hh | Reorders integer-promotion carve-out checks and adds a safe specialization for Rep = void. |
| au/constant.hh | Ensures Constant::as uses explicit-rep runtime checks and explicit-rep conversion when producing a T. |
| au/constant_test.cc | Adds tests validating explicit rep output types and overflow checking behavior for Constant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot added a non-`"` quotation mark, and MSVC choked. I took the opportunity to tidy up the comment more generally.
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
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.
From now on, whenever we do not name the repin a conversion, we'll
obtain whatever type we would naturally get if we wrote out the
conversion by hand. The main places this makes a difference are
promotable integers, and expression templates such as Eigen.
To get the mechanics right, we need to make a bunch of updates to our
:conversion_strategytarget. Many of these involve explicitlytemplating intermediate result types, instead of assuming they're the
same as the input type. But the main new interface property is that an
output rep of
voidis our signal for omitting the final castoperation.
Then, we take advantage of this by changing our implicit-rep conversion
functions in
Quantityto usevoidas the rep type. (QuantityPointfollows naturally because it uses
Quantityunder the hood.)We also need to tweak our overflow risk check in
in_impl. We need tomake the integer promotion carve-out explicit.
One common forced downstream change is that our conversion risk checkers
need to change to their explicit-rep versions in explicit-rep contexts
that we didn't formerly recognize as such. The main place this crops up
is in
Constant, where we had formerly been manifesting the constant asT{1}, and then just using implicit rep to convert to the target unit.Finally, as prep for the vector/matrix changes, I realized our ordering
in the main
PermitAsCarveOutForIntegerPromotionimplementation was notas efficient as it could be. I decided to move the
is_integralcheckscloser to the top, as they should be a lot more common than promotion
and unsigned checks, which don't really even make sense for non-integral
types.
Helps #484. This PR is a major unblocker for Eigen being usable and
performant.