Skip to content

Change implicit rep to natural output type#685

Merged
chiphogg merged 8 commits into
mainfrom
chiphogg/implicit-rep-void#484
Jun 17, 2026
Merged

Change implicit rep to natural output type#685
chiphogg merged 8 commits into
mainfrom
chiphogg/implicit-rep-void#484

Conversation

@chiphogg

@chiphogg chiphogg commented Jun 11, 2026

Copy link
Copy Markdown
Member

From now on, whenever we do not name the rep in a conversion, we'll
obtain whatever type we would naturally get if we wrote out the
conversion by hand. The main place this makes a difference today is
promotable integers. In the future, this will be critical for libraries
with expression templates, particularly 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.

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_strategy so conversions can model intermediate/output types correctly, and interpret NewRep = void as “no final cast; return natural output type”.
  • Update Quantity implicit-rep conversion paths to use void output rep and adjust overflow-risk logic to explicitly account for integer-promotion carve-outs.
  • Update Constant to 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.

Comment thread au/conversion_strategy.hh
@chiphogg chiphogg marked this pull request as draft June 11, 2026 15:54
chiphogg and others added 5 commits June 11, 2026 12:04
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.
@chiphogg chiphogg marked this pull request as ready for review June 11, 2026 18:08

@hoffbrinkle hoffbrinkle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main places this makes a difference are
promotable integers, and expression templates such as Eigen.

Do we just want to drop the ", and expression templates such as Eigen" in the commit message? I'm thinking we don't really support that yet (as of the current state of the repository). When we do, that'll just be "the" behavior.

Comment thread au/constant_test.cc
Comment on lines +338 to +341
// The following must not compile. Uncomment inside the scope to check.
{
// X.as<uint8_t>(bits);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR: I feel like we have a few of these sprinkled throughout our tests now. Should we provide some sort of list or even automated patch type script to check these as part of our release process?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, I'm inclined to have Claude Code just uncomment them all one at a time, build, and produce a report on what kind of error message we saw for each.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLMs aren't super reliable for things we want to repeat reliably. What an LLM would be good at is writing the tooling to support said strategy reliably. For instance, we could create a series of patches, with a script that applies the patch, verifies the test failure, un-applies the patch, rinse and repeat.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree. I suppose the sweet spot would be to have Claude whip up an actual death test mechanism, that can test compile time failure, and match on expected error text.

Comment thread au/no_wconversion_test.cc
@chiphogg

Copy link
Copy Markdown
Member Author

The main places this makes a difference are
promotable integers, and expression templates such as Eigen.

Do we just want to drop the ", and expression templates such as Eigen" in the commit message? I'm thinking we don't really support that yet (as of the current state of the repository). When we do, that'll just be "the" behavior.

The Eigen use case is such a critical motivator for this feature that I still wanted to mention it. I edited it, and I hope the new wording improves on the old wording.

@chiphogg chiphogg requested a review from hoffbrinkle June 17, 2026 01:46

@hoffbrinkle hoffbrinkle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Eigen use case is such a critical motivator for this feature that I still wanted to mention it. I edited it, and I hope the new wording improves on the old wording.

Works better for me!

Comment thread au/constant_test.cc
Comment on lines +338 to +341
// The following must not compile. Uncomment inside the scope to check.
{
// X.as<uint8_t>(bits);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLMs aren't super reliable for things we want to repeat reliably. What an LLM would be good at is writing the tooling to support said strategy reliably. For instance, we could create a series of patches, with a script that applies the patch, verifies the test failure, un-applies the patch, rinse and repeat.

@chiphogg chiphogg merged commit 8cd8105 into main Jun 17, 2026
30 checks passed
@chiphogg chiphogg deleted the chiphogg/implicit-rep-void#484 branch June 17, 2026 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants