[clang-tidy] Noisy cppcoreguidelines-narrowing-conversions warning for types subject to arithmetic conversion

[chrchr-github]

void f(short& a, short b, short c) {
    a = b + c;
}

<source>:2:9: warning: narrowing conversion from 'int' to signed type 'short' is implementation-defined [bugprone-narrowing-conversions]
    2 |     a = b + c;
      |

https://godbolt.org/z/913zov7vz
Maybe there could be an option to suppress the warning in case all types involved are the same? I don't see how adding a cast would improve the example.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: None (chrchr-github)

~~~c++ void f(short& a, short b, short c) { a = b + c; } ~~~ ~~~ <source>:2:9: warning: narrowing conversion from 'int' to signed type 'short' is implementation-defined [bugprone-narrowing-conversions] 2 | a = b + c; | ~~~ https://godbolt.org/z/913zov7vz Maybe there could be an option to suppress the warning in case all types involved are the same? I don't see how adding a cast would improve the example.

[HerrCai0907]

overflow of b+c will cause UB in this example.
I agree with you maybe we cannot improve the expression by add explicit cast. But I think the cppcoreguidelines wants to improve it by gsl, it will check the range firstly.

[chrchr-github]

If the intention is to recommend gsl, it should be mentioned here: https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/narrowing-conversions.html
But I doubt many people will want to add gsl::narrow_cast here.

[JonasToth]

The point is, that the Arithmetic looks like happening on short, but it doesn't. The conversions go to int (promotion) and then narrow again.
Arithmetic should be done on at least int as this is the native cpu register size.

I know the check is extremely annoying if a lot of short Arithmetic is done, but I believe it points out a real issue of the code.

[JonasToth]

I think the documentation should mention the weird conversions Integers go through for your code example.

[JonasToth]

@EugeneZelenko why did you add it as false positive? The narrowing conversion does happen in this case, it is not well explained though.
Is it ok if I remove the label again? I think the issue here is more about a bad explanation why it's narrowing even if it doesn't look like it.

[JonasToth]

If the intention is to recommend gsl, it should be mentioned here: https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/narrowing-conversions.html

Both gsl::narrow<short>() and gsl::narrow_cast<short>() would be the recommended way to silence this issue. Just inserting these casts is tricky. For a more complex arithmetic expression, the conversions happen along the whole tree of evaluations, and that becomes messy quickly.

But I doubt many people will want to add gsl::narrow_cast here.

My gut feeling and anecdotal evidence from talking with other developers suggests, that most people don't fully understand whats going on here and might be even surprised there could be an issue at all.
I created a PR to improve the documentation, please provide feedback if you think the new example is helpful.
#118209