hicpp-signed-bitwise is verbose with integer promotions

[Quuxplusone]

Bugzilla Link	PR45046
Status	NEW
Importance	P enhancement
Reported by	Pavel Kryukov (pavel.kryukov@phystech.edu)
Reported on	2020-02-27 07:53:27 -0800
Last modified on	2021-08-16 05:28:49 -0700
Version	unspecified
Hardware	PC All
CC	alexfh@google.com, alx.manpages@gmail.com, djasper@google.com, eugene.zelenko@gmail.com, klimek@google.com, N.James93@hotmail.co.uk, pavel.kryukov@phystech.edu
Fixed by commit(s)
Attachments	tidy-bug-3.cpp (742 bytes, text/x-csrc)
Blocks
Blocked by
See also

Created attachment 23180
Byte swapping code example

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

[Quuxplusone]

Attached tidy-bug-3.cpp (742 bytes, text/x-csrc): Byte swapping code example

[Quuxplusone]

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

[Quuxplusone]

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

[Quuxplusone]

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

[Quuxplusone]

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.