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

Add and document explicit-rep conversion checkers #347

Merged
merged 3 commits into from
Dec 9, 2024

Conversation

chiphogg
Copy link
Contributor

@chiphogg chiphogg commented Dec 8, 2024

The core enabling feature is the new "static cast checkers" library
target. For each source and destination type, this target provides
separate checks for overflow and truncation when static casting.

We continue with our usual policy of treating floating point types as
"value preserving". I did initially explore the possibility of treating
large integer inputs as "truncating" when converted to a floating point
type that can't represent them exactly. However, I found this made the
library somewhat harder to reason about, for questionable benefit.
Additionally, I think it is fair to assume that users intentionally
entering the floating point domain have already accepted a kind of
"magnitude based reasoning", and trying to split hairs about preserving
exact integer values just felt too cute.

With these static cast checkers in hand, the explicit-rep runtime
conversion checkers become simple. We check the static cast to the
common type, the unit conversion, and the final narrowing static cast to
the destination type.

To figure out how to write all these functions, I used some "fuzz-ish"
utilities, which generated random values of various integral and
floating point types, and performed various explicit-rep conversions. I
checked that the round-trip conversion changed the value if-and-only-if
is_conversion_lossy was true. After also taking intermediate sign
flips into account (to handle some signed/unsigned conversion edge
cases), I got to a point where integral-to-integral conversions always
gave the right result. This gives me confidence in the overall
approach. When floating point values came into the picture, I wasn't
able to design a fully satisfactory policy to avoid both false positives
and false negatives. However, I did get to a point where the kinds of
errors I saw were ones I found acceptable, relating to "the usual
floating point error". This was also what led me to embrace the policy
of simply treating floating point destination types as value-preserving,
consistent with the rest of the library. This machinery is not ready
for prime time, but I posted it for posterity in the abandoned PR #346.

I also updated the documentation, including making the floating point
policy explicit.

Fixes #110.

The core enabling feature is the new "static cast checkers" library
target.  For each source and destination type, this target provides
separate checks for overflow and truncation when static casting.

We continue with our usual policy of treating floating point types as
"value preserving".  I did initially explore the possibility of treating
large integer inputs as "truncating" when converted to a floating point
type that can't represent them exactly.  However, I found this made the
library somewhat harder to reason about, for questionable benefit.
Additionally, I think it is fair to assume that users intentionally
entering the floating point domain have already accepted a kind of
"magnitude based reasoning", and trying to split hairs about preserving
exact integer values just felt too cute.

With these static cast checkers in hand, the explicit-rep runtime
conversion checkers become simple.  We check the static cast to the
common type, the unit conversion, and the final narrowing static cast to
the destination type.

To figure out how to write all these functions, I used some "fuzz-ish"
utilities, which generated random values of various integral and
floating point types, and performed various explicit-rep conversions.  I
checked that the round-trip conversion changed the value if-and-only-if
`is_conversion_lossy` was true.  After also taking intermediate sign
flips into account (to handle some signed/unsigned conversion edge
cases), I got to a point where integral-to-integral conversions always
gave the right result.  This gives me confidence in the overall
approach.  When floating point values came into the picture, I wasn't
able to design a fully satisfactory policy to avoid both false positives
and false negatives.  However, I did get to a point where the kinds of
errors I saw were ones I found acceptable, relating to "the usual
floating point error".  This was also what led me to embrace the policy
of simply treating floating point destination types as value-preserving,
consistent with the rest of the library.  This machinery is not ready
for prime time, but I may post it as an abandoned draft PR for
posterity.

I also updated the documentation, including making the floating point
policy explicit.

Fixes #110.
@chiphogg chiphogg added the release notes: ✨ lib (enhancement) PR enhancing the library code label Dec 8, 2024
@chiphogg chiphogg requested a review from geoffviola December 8, 2024 21:06
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.

Implementation, tests, and documentation looks good. Thanks!

// Check conversion for overflow (new rep).
template <typename TargetRep, typename U, typename R, typename TargetUnitSlot>
constexpr bool will_conversion_overflow(Quantity<U, R> q, TargetUnitSlot target_unit) {
// Someday, we would like a more efficient implementation --- one that simply computes, at
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can we make this comment a TODO with an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

SIGNED_TO_UNSIGNED,
SIGNED_TO_SIGNED,
FLOAT_TO_ANYTHING,
UNEXPLORED,
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Is there more documentation on UNEXPLORED? Is it the same UNEXPLORED as below? Do any unit tests cover it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment to each of them. They are separate, but they play an identical role.

@chiphogg chiphogg merged commit 9aaeb79 into main Dec 9, 2024
13 checks passed
@chiphogg chiphogg deleted the chiphogg/explicit-rep-checkers#110 branch December 9, 2024 15:30
preferred error handling mechanism (exceptions, optional, return codes, and so on). See our
[overflow guide](../discussion/concepts/overflow.md#check-at-runtime) for more details.

We provide one checkers for overflow, truncation, and general lossiness (which combines both).
Copy link
Contributor

Choose a reason for hiding this comment

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

We provide individual checkers for overflow and truncation, as well as a checker for
general lossiness (which combines both).

Comment on lines +437 to +438
conversion will truncate. For example, if the target unit is `feet`, then `inches(61)` _would_
truncate, but `inches(60)` would _not_ truncate. Users can also provide an "explicit rep" template
Copy link
Contributor

Choose a reason for hiding this comment

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

I found this specific example required some additional thinking when read it. Specifically, I was trying to figure out what the significant of 5 feet was (I was trying to figure out a Rep) before I even considered inches(61).

For example, if the target unit is `feet`, then `inches(13)` and `inches(11)`
_would_ truncate, but `inches(12)` would _not_ truncate.

The big thing is adding both and "under" and an "over" truncation to get my brain in that mode; having inches(61) and inches(59) likely would've tripped my brain to think correctly, but I figure using 1 foot would help that even more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Too bad these comments didn't make it in time for the initial review! These are great suggestions. I created #351 to implement them, which means that thankfully, they'll make it for the 0.4.0 version of the doc website.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: ✨ lib (enhancement) PR enhancing the library code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide runtime convertibility checks
3 participants