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

Bug: Overflow in comparison ops #614

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

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
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.

1 participant