hicpp-signed-bitwise is verbose with integer promotions

[pavelkryukov]

Bugzilla Link	45046
Version	unspecified
OS	All
Attachments	Byte swapping code example
CC	@alejandro-colomar,@EugeneZelenko,@pavelkryukov

Extended Description

Hi

Attached code does not use signed types at all, however, Clang-Tidy outputs following:

$ clang-tidy-10 --checks='hicpp-signed-bitwise' ./tidy-bug-3.cpp -- -std=c++17 69 warnings generated. /tidy-bug-3.cpp:9:9: warning: use of a signed integer operand with a binary bitwise operator [hicpp-signed-bitwise] value |= uint16_t( el) << shift; ^

Godbolt: https://godbolt.org/z/mfhRmw

[286053d7-b879-4c05-bec3-f56f0afc86d1]

The RHS doesn't intervene in the resulting type. See the standard:

[[ C17::6.5.7: Bitwise shift operators

[...]

Semantics 3 The integer promotions are performed on each of the operands. The type of the result is that of the promoted left operand. If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined.

4 The result ofE1 << E2 is E1 left-shifted E2 bit positions; vacated bits are filled with zeros. [...]. If E1 has a signed type and nonnegative value, and E1×2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined. ]]

And see a related StackOverflow question: https://stackoverflow.com/questions/16880187/weird-integral-promotions-with-left-shift-operator

Conclusion:

Unsigned integral types shorter than int will be promoted to int, no matter what is on the RHS. But since that conversion keeps the positive signedness of the to-be-shifted value, the result is defined as long as you don't shift enough to mess with the sign bit of 'int'.

If you may shift by more than 15, it may be unsafe, and you better cast to 'unsigned'.

IMO, this automatic promotion doesn't make any sense, and is bug-prone, but it is what we have.

[pavelkryukov]

I'm not entirely sure if its correct to promote an unsigned short to a signed int

According to N4713, 7.6 promoting to signed int is legal:

A prvalue of an integer type other than bool, char16_t, char32_t, or wchar_t whose integer conversion rank (6.7.4) is less than the rank of int can be converted to a prvalue of type int if int can represent all the values of the source type; otherwise, the source prvalue can be converted to a prvalue of type unsigned int.

[llvmbot]

Further reduced test with AST visible https://godbolt.org/z/8WRRRQ

Looking at the AST the error comes about because the return type of left shifting an unsigned short by an unsigned is int. Don't know the standard well enough but this seems interesting

The return type is the type of the left operand after integral promotions.

So my guess is the unsigned short gets promoted to an integer which is ultimately the cause of this warning. I'm not entirely sure if its correct to promote an unsigned short to a signed int, instead of unsigned int, when the RHS is unsigned int.

If someone with more in depth understanding can weigh in that'd be great

[pavelkryukov]

Reduced test case: https://godbolt.org/z/wsB2Vg