Skip to content

Bug: Overflow in comparison ops#614

Closed
burnpanck wants to merge 2 commits intompusz:masterfrom
burnpanck:bug/overflow-in-comparison-operator
Closed

Bug: Overflow in comparison ops#614
burnpanck wants to merge 2 commits intompusz:masterfrom
burnpanck:bug/overflow-in-comparison-operator

Conversation

@burnpanck
Copy link
Contributor

Found this while working on #580. This one is in some sense related, but even a fix to #580 doesn't fix this one. In the current implementation, both operands are converted to their "common_type", which is currently defined as quantity<gcd_unit, common_repr>. If the conversion ratio between the two has a large numerator or denominator, the conversion to the gcd unit may easily overflow. These are the same situations in which the direct conversion currently overflows, but the result would still fit (if implemented differently). The conversion to gcd on the other hand would potentially require a significantly larger representations.

In the first commit here, I add a test case, where such a comparison overflow happens even for int64. Interestingly enough, I believe there is already a test case which overflows for int32. It just happens that, due to the internal conversion to the gcd unit currently being done in 64bit, it doesn't immediately overflow (which would fail in the constexpr context here), and the subsequent static_cast to the output overflows silently. Because the overflow happens "on both sides" of the comparison operator, the ultimate comparison results happens to turn out as expected here.

@burnpanck
Copy link
Contributor Author

I believe, when combined with my fix #615 for #580, an acceptable solution here may be the following: Instead of converting to the gcd unit between the two operands, convert to the "smaller" unit of the two operands. With #615, the conversion is "safe" in the sense that it accurately converts without overflowing - unless the resulting value does not fit the representation type. I think this may be acceptable: a programmer using integer types for physical quantities will have a rough understanding of the range of their quantities, and thus can easily evaluate if those values risk overflowing the range of their representation in either of the two unit. In contrast, the current solution requires reasoning about the "gcd" unit of the two, which may be far less intuitive (I know US tonne and the metric tonne are roughly the same; yet their gcd is three orders of magnitude smaller).

Another solution would be to make direct use of the tooling developed for #615: Double-width integers. As long as the ratio can be represented with intmax, then the conversion of an intmax number to the gcd unit will take at most twice as many bits as intmax provides. The fixed-point implementation internally synthesizes a double-width integer if needed, in order to make that conversion safe. Now that I think of this, it may in fact be the better option here, given that we anyway defer to double-width integers internally.

@burnpanck burnpanck force-pushed the bug/overflow-in-comparison-operator branch from 832d2cd to 10444cc Compare September 16, 2024 15:29
mpusz pushed a commit that referenced this pull request Mar 7, 2026
Introduce a fixed-point implementation for unit conversions involving
integer representations, avoiding loss of significant digits that
previously occurred when the conversion factor was not a whole number.

New files:
- src/core/include/mp-units/bits/fixed_point.h: double_width_int<T> and
  fixed_point<T,n> types for exact rational scaling of integer values.
  Uses __int128 when available (__SIZEOF_INT128__) for 64-bit integers.
- src/core/include/mp-units/framework/scaling.h: public scaling_traits<>
  customization point and scale<To>(M, value) free function. Provides
  built-in specializations for floating-point and integer-like types.
- test/static/fixed_point_test.cpp: static assertions for the new types.
- test/runtime/fixed_point_test.cpp: runtime arithmetic edge-case tests.

Modified:
- sudo_cast.h: replace hand-rolled conversion_value_traits / sudo_cast_value
  machinery with a single scale<To::rep>(c_mag, ...) call.
- representation_concepts.h: add MagnitudeScalable concept; replace
  ComplexScalar with HasComplexOperations (which is its definition).
- customization_points.h: add unspecified_rep tag and declare the primary
  scaling_traits<> template.
- framework.h / CMakeLists.txt: wire in the new headers.
- hacks.h: add MP_UNITS_DIAGNOSTIC_IGNORE_PEDANTIC and
  MP_UNITS_DIAGNOSTIC_IGNORE_SIGN_CONVERSION macros.
- example/measurement.cpp: add scaling_traits specializations for
  measurement<T> to demonstrate the customization point.
- test/static/{international,usc}_test.cpp: disable two tests that are
  blocked on issue #614.

Co-authored-by: Tobias Hanhart <burnpanck@users.noreply.github.com>
@mpusz mpusz marked this pull request as ready for review March 7, 2026 19:12
mpusz added a commit that referenced this pull request Mar 7, 2026
* Fix #580: use fixed-point arithmetic for integer unit conversions

Introduce a fixed-point implementation for unit conversions involving
integer representations, avoiding loss of significant digits that
previously occurred when the conversion factor was not a whole number.

New files:
- src/core/include/mp-units/bits/fixed_point.h: double_width_int<T> and
  fixed_point<T,n> types for exact rational scaling of integer values.
  Uses __int128 when available (__SIZEOF_INT128__) for 64-bit integers.
- src/core/include/mp-units/framework/scaling.h: public scaling_traits<>
  customization point and scale<To>(M, value) free function. Provides
  built-in specializations for floating-point and integer-like types.
- test/static/fixed_point_test.cpp: static assertions for the new types.
- test/runtime/fixed_point_test.cpp: runtime arithmetic edge-case tests.

Modified:
- sudo_cast.h: replace hand-rolled conversion_value_traits / sudo_cast_value
  machinery with a single scale<To::rep>(c_mag, ...) call.
- representation_concepts.h: add MagnitudeScalable concept; replace
  ComplexScalar with HasComplexOperations (which is its definition).
- customization_points.h: add unspecified_rep tag and declare the primary
  scaling_traits<> template.
- framework.h / CMakeLists.txt: wire in the new headers.
- hacks.h: add MP_UNITS_DIAGNOSTIC_IGNORE_PEDANTIC and
  MP_UNITS_DIAGNOSTIC_IGNORE_SIGN_CONVERSION macros.
- example/measurement.cpp: add scaling_traits specializations for
  measurement<T> to demonstrate the customization point.
- test/static/{international,usc}_test.cpp: disable two tests that are
  blocked on issue #614.

Co-authored-by: Tobias Hanhart <burnpanck@users.noreply.github.com>

* Fix value_Type typo in floating_point_scaling_factor_type specialization

The partial specialization for types with a nested value_type used
'value_Type' (capital T) instead of 'value_type', making the entire
specialization dead code as the requires-clause could never be satisfied.

Also fix 'mantiassa' -> 'mantissa' in the adjacent comment.

* Fix docstring typos in scaling_traits documentation

- 'quantitiy' -> 'quantity'
- 'dictatet' -> 'dictated'
- 'convetrible' -> 'convertible'
- 'implemenation' -> 'implementation'
- 'availabe' -> 'available'

* Fix conflict resolution error: keep ComplexScalar name from master

When resolving the merge conflict in representation_concepts.h, the
PR's renamed version of the concept ('HasComplexOperations') was used
instead of master's established name ('ComplexScalar'). The two concepts
are semantically equivalent — burnpanck simply renamed it in his branch.

Revert to the canonical 'ComplexScalar' name while retaining the new
'MagnitudeScalable' concept which was the actual addition from the PR.

* Fix measurement.cpp: remove duplicate class definition from merge

The PR branched from a version where measurement<T> was defined inline
in measurement.cpp. Master later moved the class to example/include/
measurement.h and changed measurement.cpp to #include that header.

The squash merge therefore introduced a duplicate definition: the class
from the header and the PR's inline class were both visible, causing
an 'ambiguous reference' error. Remove the now-redundant inline class;
the scaling_traits specializations added by the PR work correctly with
the class from measurement.h.

* style: pre-commit

* docs: chapters anchors improved in the "custom representation" chapter

* docs: value conversions chapter improved

* refactor: scaling support refactored

* fix: clang-16 crash fixed

* docs: `measurement` example documentation updated to match changes

* fix: use exact wide-integer arithmetic for rational unit conversions on all platforms

On ARM / Apple Silicon, long double == double (64-bit mantissa).  The old
fixed_point<T>(long double) initialiser lost ~12 bits of precision for 64-bit
integer types when representing the scaling ratio, producing an error of ~49
units for the 10/9 (degree → gradian) conversion with a 10^18 input value.

Fix by splitting the integer-path else-branch into two cases:

  • Pure rational M (is_integral(M * (denominator(M) / numerator(M))) == true):
    use (value * numerator) / denominator via double_width_int_for_t<> arithmetic.
    This is exact on every platform regardless of long double width.

  • Irrational M (involves π etc.): keep the long double fixed_point approximation.
    These conversions are inherently approximate; small values still produce correct
    truncated results on all platforms.

Update the test comment to reflect the new exact-arithmetic path.

Fixes CI failures on clang-18/ARM and apple-clang-16.

* fix: replace floating-point TeX-point test with exact integer equivalent

72.27 is not exactly representable as double (it rounds to 72.2699...96).
Multiplying by the conversion factor 100/7227 via long double gives a result
≥ 1.0 on x86 (80-bit long double, 64-bit mantissa) only by chance, but
0.99999...978 on ARM / Apple Silicon where long double == double (52-bit).

The correct mathematical statement is: 7227 tex_point = 100 inch (exact
rational relationship).  Use that integer form instead of the inexact 72.27
double literal so the test is correct and platform-independent.

---------

Co-authored-by: Tobias Hanhart <burnpanck@users.noreply.github.com>
@mpusz
Copy link
Owner

mpusz commented Mar 8, 2026

Fixed!

@mpusz mpusz closed this Mar 8, 2026
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.

2 participants