CppCoreGuidelines icon indicating copy to clipboard operation
CppCoreGuidelines copied to clipboard

Suggest gsl::narrow/gsl::narrow_cast for float/double to integral conversions in ES.46

Open sunnychatterjee opened this issue 5 years ago • 3 comments

One of the things ES.46 touches upon is the following:

Enforcement A good analyzer can detect all narrowing conversions. However, flagging all narrowing conversions will lead to a lot of false positives. Suggestions:

Flag all floating-point to integer conversions (maybe only float->char and double->int. Here be dragons! we need data).

We have recently ran into an issue where directly casting a negative double to an unsigned integral type results in inconsistent code generation when targeting a specific architecture. Therefore, we want to implement a static check that flags all conversions from negative double/float to unsigned integral types and suggests to use gsl::narrow or gsl::narrow_cast. My hope is we can then update the existing implementation of gsl::narrow/gsl::narrow_cast to guard against such conversions by first going from float/double to signed integral type and then convert the result to unsigned, guarding against data loss during the process.

Can we improve the documentation of ES.46 such that it suggests people to use gsl::narrow and gsl::narrow_cast when doing float/double to integral conversions?

sunnychatterjee avatar Dec 04 '20 23:12 sunnychatterjee

The guideline does include the line

We also include lossy arithmetic casts, such as from a negative floating point type to an unsigned integral type:

followed by example code which seems to show the issue you're describing.

I believe the intention of the guideline is that gsl::narrow handles this case in the way you'd expect. i.e. if your value is negative it's going to raise an error (of some sort) and the microsoft implementation seems to reflect that (from looking at the code. I haven't tested) https://github.com/microsoft/GSL/blob/master/include/gsl/narrow .

Can you clarify what you're asking for because as far as I can tell what you're asking for already exists?

shaneasd avatar Dec 28 '20 04:12 shaneasd

Yes, data would be great. The very first C++ (and C with classes) got away with banning long->char and float->char.

BjarneStroustrup avatar Jun 24 '21 14:06 BjarneStroustrup

Editors call: Are you suggesting that we require narrow or narrow_cast for all float->unsigned conversions, and that narrow gives "the right" value (as if it was an integer value) and narrow_cast gives a run-time error?

Can we improve the documentation of ES.46 such that it suggests people to use gsl::narrow and gsl::narrow_cast when doing float/double to integral conversions?

We think the guideline does say that, including the part about "We also include lossy arithmetic casts, such as from a negative floating point type to an unsigned integral type"... can you elaborate what wording change you would like to see? Is the request to make the quoted Enforcement unconditional, that is:

  • Flag all floating-point to integer conversions.

hsutter avatar Jun 24 '21 18:06 hsutter