Skip to content
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

Explicitly specify QuantityPoint calc type #234

Merged
merged 1 commit into from
May 2, 2024
Merged

Conversation

chiphogg
Copy link
Contributor

@chiphogg chiphogg commented May 2, 2024

We want to carry out our intermediate computations in the common type of
the old and new rep, generally. However, we already know from #222 that
if the new rep is a signed integer, and the old rep is unsigned, we need
to be careful to use a signed type for intermediate computations.

To preserve this behaviour but restore correct behavoiur for other cases
(namely, #233), we add a new type trait for this intermediate type. It
basically boils down to the common type, but with an explicit carve-out
to keep #222 fixed. We explicitly cast both participants to this new
type-for-intermediate-computations, and then cast to the destination rep
at the end.

Compile time tests showed no discernible impact at all. This change has
also passed all Aurora-internal builds.

Fixes #233.

We want to carry out our intermediate computations in the common type of
the old and new rep, generally.  However, we already know from #222 that
if the new rep is a signed integer, and the old rep is unsigned, we need
to be careful to use a signed type for intermediate computations.

To preserve this behaviour but restore correct behavoiur for other cases
(namely, #233), we add a new type trait for this intermediate type.  It
basically boils down to the common type, but with an explicit carve-out
to keep #222 fixed.  We explicitly cast both participants to this new
type-for-intermediate-computations, and then cast to the destination rep
at the end.

Fixes #233.
Copy link
Contributor

@geoffviola geoffviola left a comment

Choose a reason for hiding this comment

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

This case is tricky. Changes and tests look good!

au/quantity_test.cc Show resolved Hide resolved
@chiphogg chiphogg merged commit 30440ed into main May 2, 2024
10 checks passed
@chiphogg chiphogg deleted the chiphogg/calc-rep#233 branch May 2, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: 🐛 lib (bugfix) PR fixing a defect in the library code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QuantityPoint: Value is forced into target Rep before unit conversion
2 participants