CppCoreGuidelines icon indicating copy to clipboard operation
CppCoreGuidelines copied to clipboard

C++ integer promotion defeats "ES.101: Use unsigned types for bit manipulation" on small integer types

Open ilelann opened this issue 4 years ago • 4 comments

I just fell on "Clang-Tidy: Use of a signed integer operand with a binary bitwise operator" warning. Point is, I was writing 8bit unsigned integer expressions from my point of view, but expression below can involve signed arithmetic: uint8_t{} | uint8_t{} | uint8_t{}

Which is not obvious.

Because of [over.built] and [conv.prom] sections, below assert is not guaranted to hold: static_assert(std::is_same_v<decltype(std::declval<T>() | std::declval<T>()), T>) And more importantly, this one does not hold either: static_assert(std::is_signed_v< decltype(std::declval<T>() | std::declval<T>())> == std::is_signed_v<T>)

It currently fails on major implementations for uint8_t, uint_fast8_t, etc. My limited understanding of the standard is that it could fail for any integer type smaller than int.

Technically the rule is correct, and I am not saying it should be changed to forbid bitwise ops on small ints. But I think it reveals a quite uncomfortable situation that deserves a note.

ilelann avatar Oct 03 '20 14:10 ilelann

Similar issue: #1120, integer promotion is kind of a problem :see_no_evil:

JonasToth avatar Oct 11 '20 14:10 JonasToth

Arguably clang-tidy (or the guideline) should be a bit smarter. Although the operands will get promoted to signed int the values will still be in the range 0 to 255 and so the sign bit won't be set in any of the operands or in the result of the | operator.

jwakely avatar Oct 11 '20 16:10 jwakely

@jwakely Is there any reason to expose int operator|(uint8_t, uint8_t) instead of unsigned int operator|(uint8_t, uint8_t) ? [conv.prom] says

can be converted

so for me a choice is possible. All compilers seems to have picked the least intuitive type to me, which makes me curious.

Note 1 I think it is even allowed to expose uint8_t operator|(uint8_t, uint8_t) but that might be an arguable choice for operator+ and I guess you are supposed to always pick the same common type.

Note 2 From my limited testing with uint8_t, with gcc/clang/msvc/icc

  • casting to uint8_t at each stage produce the best assembly overall
    • only really changes for msvc and icc
    • marginally changes for gcc
    • does not change at all for clang
  • unsurprisingly, casting to unsigned int changes nothing in assembly, in both optimized builds and default ones

See https://godbolt.org/z/jarff5

ilelann avatar Oct 11 '20 19:10 ilelann

No choice is possible, promotion to int is required.

jwakely avatar Oct 11 '20 19:10 jwakely