docs: Rational's operators saturate and log; four passages still described UB - #266
Merged
Conversation
…ribed UB rational.hpp's +, - and * detect an overflow before forming the result, saturate toward the correctly-signed ±INT64_MAX/1, and log at error naming checkedAdd/checkedSub/checkedMul. Four passages across three documents still described the pre-saturation behaviour as undefined behaviour, and rational.md made both claims at once: its "Overflow & value-range envelope" section and its Limitations bullet said silent UB, while line 200 -- a few dozen lines below the first -- says in bold "The operators saturate; they never overflow." Corrected in README.md, docs/spec/util/quantity_type.md, and rational.md (the operator table row, the envelope section, the "garbage Rational / UBSan trap" paragraph, and the Limitations bullet). The correction is not simply "it saturates, all is well". A saturated result is still wrong -- it is clamped and inexact, and nothing in the return type distinguishes it from an exact one. What changed is the failure mode: bounded and logged rather than undefined and silent, with checkedAdd/checkedSub/ checkedMul available for a caller that must branch on it. The text says that rather than replacing one over-simplification with another. The transition is recorded in the tests, which is where the drift shows: tests/test_rational_checked.cpp:154 opens the saturation cases with "Before these, every case below was undefined behaviour". The behaviour was changed and tested; the specs were never updated to match. Verified against the code rather than the issue: operator*= calls mulWouldOverflow() then reportOverflow() + saturateToward(), operator+= the same via addWouldOverflow(), and reportOverflow() logs at error. [saturate] tests pass; full suite 20657 assertions / 1170 cases; spec-citation lint clean. Closes #221
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Closes #221.
The defect
rational.hpp's+,-and*detect an overflow before forming theresult, saturate toward the correctly-signed
±INT64_MAX/1, and log aterrornaming
checkedAdd/checkedSub/checkedMul. Four passages across threedocuments still described the pre-saturation behaviour as undefined behaviour.
rational.mdmade both claims at once — its "Overflow & value-range envelope"section and its Limitations bullet described silent UB, while line 200, a few
dozen lines below the first, says in bold:
A reader deciding whether they need the checked variants got the wrong mental
model twice in the same file.
What changed
Corrected in
README.md,docs/spec/util/quantity_type.md, andrational.md(operator table row, envelope section, the "garbage
Rational/ UBSan trap"paragraph, and the Limitations bullet).
The correction is not "it saturates, all is well." A saturated result is
still wrong — clamped and inexact, and nothing in the return type distinguishes
it from an exact one. What changed is the failure mode: bounded and logged
rather than undefined and silent, with the checked variants available for a
caller that must branch on it. I wrote it that way rather than swapping one
over-simplification for another — the old text at least conveyed "don't rely on
this", and a naive fix would have lost that.
Verified against the code, not the issue
operator*=→mulWouldOverflow()→reportOverflow()+saturateToward()operator+=→addWouldOverflow()→ samereportOverflow()logs at error, naming the checked alternativesWhere the drift shows
tests/test_rational_checked.cpp:154opens its saturation cases with:The behaviour was changed and tested; the specs were simply never updated to
match. That's the whole issue in one line.
[saturate]testsscripts/check_spec_citations.shDocumentation only — no code changes.