fixed integer overflow when wrapping integer param

[johnmcfarlane]

Thanks for the submission. On brief inspection it looks very reasonable, though I'm a little hazy on the implementation details. However, a little more detail would be appreciated. An example (or better still a test) showing where bug-free code resulted in an overflow would be really helpful.

[celestialkylin]

Ah sorry, I haven't provided any details, please take a look at this example code chunk:

cnl::scaled_integer<int64_t, cnl::power<-20, 2>> number; number = 10000; std::cout << number << std::endl;

I encoutered an issue where the output is "1808" instead of "10000". the discrepancy occurred because during conversion, the result type was not convert to "int64_t", but remains as "int", causing an overflow in the result after scaled.

My submission tried to fix this issue.

[johnmcfarlane]

If my memory serves me correctly, that will pessimise the code by always casting to a wider type, regardless of whether overflow or underflow are risks. One way to indicate whether this is the case would be to build the test suite with and without the change and observe differences in the binaries generated.

If you want to avoid the risk of overflow that you are observing, try replacing a native integer with an elastic_integer. If you want to detect cases of overflow reliably, consider using overflow_integer instead (or as well). Composing types from different APIs in this way is the intended way to get the functionality you're after.

I'd greatly appreciate you trying out these things to make sure I'm correct. There's a possibility this is a genuine bug. Also, any suggestions on how I could improve the documentation, including in-code comments would be a great help.

On Sat 27 Apr 2024, 06:05 celestialkylin, @.***> wrote:

Ah sorry, I haven't provided any details, please take a look at this example code chunk:

cnl::scaled_integer<int64_t, cnl::power<-20, 2>> number; number = 10000; std::cout << number << std::endl;

I encoutered an issue where the output is "1808" instead of "10000". the discrepancy occurred because during conversion, the result type was not convert to "int64_t", but remains as "int", causing an overflow in the result after scaled.

My submission tried to fix this issue.

— Reply to this email directly, view it on GitHub https://github.com/johnmcfarlane/cnl/pull/1048#issuecomment-2080364976, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFTGN4WW2CXNQWBL4QVST3Y7MWYFAVCNFSM6AAAAABG2O3GWOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBQGM3DIOJXGY . You are receiving this because you commented.Message ID: @.***>

[celestialkylin]

Thank you for your response.

Perhaps it's just that our priorities are different; I'm more concerned about the API not having too many hidden issues in its usage.

Going back to my example, if the code were

number = 10000ll or number = (int64_t)10000,

there wouldn't be any problems. However, in practical use, explicit conversions like these are often omitted. Your consideration also makes sense, but whether to convert to a wider type depends on the Represent Type(Rep template parameter) chosen at the time of definition. From my perspective, converting the input to the Represent Type also makes logical sense.

The above are just some of my suggestions. If you believe no changes are needed, I will close my Pull Request later. Thank you.

[johnmcfarlane]

@celestialkylin thanks again for the feedback.

I would argue that your priorities are served by CNL, but not served by scaled_integer in isolation. Early in the design of that API (when it was called fixed_point), it became apparent that while fundamental integers make it possible to represent fixed-point values, that there were some 'share edges', so to speak.

Out-of-range errors (sometimes resulting in overflow) are a much greater problem with fixed-point arithmetic when:

1. the range of the underlying type is saturated (think π with only 2 integer digits), or
2. a scaling conversion from one exponent to another occurs.

Both of these are exacerbated when combined with narrowing casts, which are sometimes implicit.

It's usually possible to design an API with a 'wide contract', i.e. one where results of all usage are defined. But sometimes, this involves is a tradeoff where quality is lower by some other measure, e.g. performance.

I'm fairly confident that different target users have contrasting priorities here, and I don't want to exclude any user in favour of any other - hence you need to use the correct type for managing overflow (which may sometimes be slower) in composition with the correct type for scaling integers.

For further details, see the question,

Q: Why doesn't cnl::scaled_integer prevent/avoid/detect overflow?

from the fixed-point overflow section of the FAQ.