"Use of a signed integer operand with a binary bitwise operator" - when using unsigned short

Question

In the following C snippet that checks if the first two bits of a 16-bit sequence are set:

bool is_pointer(unsigned short int sequence) {
  return (sequence >> 14) == 3;
}

CLion's Clang-Tidy is giving me a "Use of a signed integer operand with a binary bitwise operator" warning, and I can't understand why. Is unsigned short not unsigned enough?

Solution 1

The code for this warning checks if either operand to the bitwise operator is signed. It is not sequence causing the warning, but 14, and you can alleviate the problem by making 14 unsigned by appending a u to the end.

(sequence >> 14u)

This warning is bad. As Roland's answer describes, CLion is fixing this.

Solution 2

There is a check in clang-tidy that is called hicpp-signed-bitwise. This check follows the wording of the HIC++ standard. That standard is freely available and says:

5.6.1. Do not use bitwise operators with signed operands

Use of signed operands with bitwise operators is in some cases subject to undefined or implementation defined behavior. Therefore, bitwise operators should only be used with operands of unsigned integral types.

The authors of the HIC++ coding standard misinterpreted the intention of the C and C++ standards and either accidentally or intentionally focused on the type of the operands instead of the value of the operands.

The check in clang-tidy implements exactly this wording, in order to conform to that standard. That check is not intended to be generally useful, its only purpose is to help the poor souls whose programs have to conform to that one stupid rule from the HIC++ standard.

The crucial point is that by definition integer literals without any suffix are of type int, and that type is defined as being a signed type. HIC++ now wrongly concludes that positive integer literals might be negative and thus could invoke undefined behavior.

For comparison, the C11 standard says:

6.5.7 Bitwise shift operators

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.

This wording is carefully chosen and emphasises that the value of the right operand is important, not its type. It also covers the case of a too large value, while the HIC++ standard simply forgot that case. Therefore, saying 1u << 1000u is ok in HIC++, while 1 << 3 isn't.

The best strategy is to explicitly disable this single check. There are several bug reports for CLion mentioning this, and it is getting fixed there.

---

Update 2019-12-16: I asked Perforce what the motivation behind this exact wording was and whether the wording was intentional. Here is their response:

Our C++ team who were involved in creating the HIC++ standard have taken a look at the Stack Overflow question you mentioned.

In short, referring to the object type in the HIC++ rule instead of the value is an intentional choice to allow easier automated checking of the code. The type of an object is always known, while the value is not.

• HIC++ rules in general aim to be "decidable". Enforcing against the type ensures that a decidable check is always possible, ie. directly where the operator is used or where a signed type is converted to unsigned.
• The rationale explicitly refers to "possible" undefined behavior, therefore a sensible implementation can exclude:
  • constants unless there is definitely an issue and,
  • unsigned types that are promoted to signed types.
• The best operation is therefore for CLion to limit the checking to non-constant types before promotion.

Solution 3

I think the integer promotion causes here the warning. Operands smaller than an int are widened to integer for the arithmetic expression, which is signed. So your code is effectively return ( (int)sequence >> 14)==3; which leds to the warning. Try return ( (unsigned)sequence >> 14)==3; or return (sequence & 0xC000)==0xC000;.

Comments

[Antti Haapala -- Слава Україні]

Wow... just wow... I wonder why it isn't complaining about the sequence that is promoted to a signed int.

[autistic]

At the time of reading this question, this was my thought, too... but then the (currently accepted) other answer popped up before submitting my own answer, which leads me to believe that Oliver was right... C11/6.3.1.1p2 is your citation, btw

[Ryan Haining]

@AnttiHaapala it's a pretty weird check I agree

[SakoDaemon]

I accepted this answer because it made the warning go away, whereas the other one didn't :)

[Roland Illig]

Making a "warning go away" is not always good. In some cases (like this one) it's the warning that is wrong, not your code. Don't blindly trust the warnings. Instead, try to understand them and then decide who is wrong.

[Pnemonic]

the potential problem is when negative values are shifted right - does the CPU prepend a 0 (nothing was there before) or 1 (maintaining negativity)?

[Roland Illig]

@Pnemonic Yes, that's another area of undefined behavior. But that has nothing to do with this HICPP check, which is about the confusion between having types that allow negative values and having values that could be negative.

[mic]

To disable the rule in CLion, go to File > Settings/Preferences > Editor > Inspections, and in the right pane, select Clang-Tidy. Under Options, in the field labeled "Comma-separated list of enabled and disabled checks, write a comma and then -hicpp-signed-bitwise, and press OK.

[Roland Illig]

@mic Thanks for the workaround. That's not a proper solution though. If I disable the rule in my personal copy of the IDE, this won't prevent others from being confused as well. Therefore, the only sensible thing is to tell the distributor of this wrong check (in this case JetBrains, and further upstream Clang-Tidy) to fix this.