66

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?

SakoDaemon
  • 973
  • 1
  • 6
  • 21
  • 10
    Sounds like a bug. – Oliver Charlesworth May 17 '18 at 19:26
  • 6
    [It's getting fixed in JetBrains CLion](https://youtrack.jetbrains.com/issue/CPP-12624). There is a discussion from 2018 in [clang-tidy bug 36961](https://bugs.llvm.org/show_bug.cgi?id=36961), but it's not yet fixed. – Roland Illig Oct 08 '19 at 16:04
  • 2
    I contacted the Perforce support, asking for a clarification on this issue. Let's see what they reply. (Perforce is the maintainer of the standard that is implemented by clang-tidy, which again is used in CLion.) – Roland Illig Dec 12 '19 at 23:37
  • I would add context is everything: why do you want to know about the first two bits? What are you going to do with them? – JosephDoggie Jul 13 '23 at 19:35
  • @JosephDoggie -- what would the requested context have to do with the problem of a buggy warning? – ad absurdum Jul 13 '23 at 19:48
  • @ad absurdum -- Sometimes when you get warnings you are doing something you shouldn't be doing in the first place. – JosephDoggie Jul 13 '23 at 19:56
  • There are 2^16 possible inputs but only 4 possible outputs. It would be possible (though tiresome) to make an exhaustive table of all possible input output combinations and see if the function performed correctly. One could also just check a few important values (maybe 16 or so) and see if all 4 possible outputs are reached, and then one would know that the function worked correctly and it was safe to ignore the warning. But I guess this isn't the scope of the OP question. Still, I've done a lot of legacy programming, and rightly or wrongly, programmers ignore warnings all the time! – JosephDoggie Jul 13 '23 at 20:00
  • @JosephDoggie -- Agree that ignoring warnings is bad, and generally context is important and too frequently overlooked. It seems that _"CLion's Clang-Tidy is giving me..."_ supplies the relevant context here. But I don't see how OP's _intentions_ help understand why they are getting a warning for trying to bitshift an unsigned integer. It might help with finding alternative solutions, I suppose. – ad absurdum Jul 13 '23 at 20:06

3 Answers3

87

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.

Ryan Haining
  • 35,360
  • 15
  • 114
  • 174
31

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.
Roland Illig
  • 40,703
  • 10
  • 88
  • 121
  • 1
    the potential problem is when negative values are shifted right - does the CPU prepend a 0 (nothing was there before) or 1 (maintaining negativity)? – Pnemonic Jun 28 '20 at 08:40
  • 1
    @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_. – Roland Illig Jun 29 '20 at 18:19
  • 3
    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. – mic Nov 10 '20 at 16:59
  • 1
    @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. – Roland Illig Nov 14 '20 at 12:07
10

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

user5329483
  • 1,260
  • 7
  • 11
  • 4
    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](https://port70.net/~nsz/c/c11/n1570.html#6.3.1.1p2) is your citation, btw – autistic May 17 '18 at 20:33