hicpp-signed-bitwise - Return location of the operand (and not of the operator beginning)

[vladimir.plyashkun]

Summary

Hello!
Currently, the "hicpp/signed-bitwise" check returns the beginning of the binary/unary operator as location, which sometimes confuses users in the IDE due to incorrect highlighting.
Yes, the offset from Ranges can be used for this particular check, but i suppose better solution is to return begin location of the problematic operand instead of operator.

[njames93]

clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp

98–100

Seems pretty useless to pass the begin location of the Unary/Binary operator here.
The only source locations that are of interest is the operator location and the whole range of the SignedOperand.
Based on the issues with jetbrains rendering, I also imagine clangd would have the same issue, We'd likely want to have the diag locations as the SignedOperand begin(like this patch). Then pass the whole source range of the signed operand(like it currently is).
I'm either way on whether we would need to pass the location of the operator (BinaryOp|UnaryOp)->getOperatorLoc()

[carlosgalvezp]

Hi @vladimir.plyashkun !

I looked at the screenshot in the link you posted. What is the type of value?

Consider that if value is an uint8_t, as per the integer promotion rules, it will be promoted to (signed) int before running the bitwise operation. Therefore clang-tidy is right to point out the problem with value. Check it out:

https://godbolt.org/z/8EMcPd6rd

`-DeclStmt <line:6:5, col:22>
  `-VarDecl <col:5, col:18> col:10 y 'int':'int' cinit
    `-BinaryOperator <col:14, col:18> 'int' '&'
      |-ImplicitCastExpr <col:14> 'int' <IntegralCast>
      | `-ImplicitCastExpr <col:14> 'std::uint8_t':'unsigned char' <LValueToRValue>
      |   `-DeclRefExpr <col:14> 'std::uint8_t':'unsigned char' lvalue Var 0x55db629e40e8 'x' 'std::uint8_t':'unsigned char'
      `-IntegerLiteral <col:18> 'int' 255

[carlosgalvezp]

On the other hand, if the 0xFF becomes 0xFFu, then Clang performs an integer promotion to unsigned int instead of int, and that's why clang-tidy no longer complains.

I'm not sure that's correct behavior, however:

If int can represent the entire range of values of the original type (or the range of values of the original bit field), the value is converted to type int. Otherwise the value is converted to unsigned int.

https://en.cppreference.com/w/c/language/conversion

In this case, an int can represent fully all values of an uint8_t.

[vladimir.plyashkun]

Hi @carlosgalvezp
Yes, sorry, maybe screenshot and example from issue in our bugtracker is not the best one.
You can check it on this sample from clang-tidy tests:
The "hicpp-signed-bitwise" will point to

URes = UValue & -1;

[UValue] instead of -1, which looks confusing.

[vladimir.plyashkun]

clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp

98–100

My initial point was to preserve current behaviour as much as possible, but i agree that (BinaryOp|UnaryOp)->getOperatorLoc() is much better in this case.

[njames93]

The pre-merge bot is failing because clang-format hasn't been run. Can you please update the diff by running git clang-format first.

[carlosgalvezp]

In D131678#3716213, @vladimir.plyashkun wrote:

Hi @carlosgalvezp
Yes, sorry, maybe screenshot and example from issue in our bugtracker is not the best one.
You can check it on this sample from clang-tidy tests:
The "hicpp-signed-bitwise" will point to

URes = UValue & -1;

[UValue] instead of -1, which looks confusing.

Ah, yes that seems off, thanks for the clarification! :)

[vladimir.plyashkun]

• Executed clang-format after changes
• Return operator location (in additional range)

[njames93]

clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp

100

We still need the source range of the operand

[build]   Value <<= diagTest(1, 2, 3);
[build]         ~~~ ^

Vs

[build]   Value <<= diagTest(1, 2, 3);
[build]         ~~~ ^~~~~~~~~~~~~~~~~

[vladimir.plyashkun]

• return operand source range too

[njames93]

Would you like me to commit it on your behalf again?

[vladimir.plyashkun]

In D131678#3728412, @njames93 wrote:

Would you like me to commit it on your behalf again?

Yes, the email is the same - [e-mail]

clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp

 
 void SignedBitwiseCheck::check(const MatchFinder::MatchResult &Result) {
   const ast_matchers::BoundNodes &N = Result.Nodes;
   const auto *SignedOperand = N.getNodeAs<Expr>("signed-operand");
   assert(SignedOperand &&
          "No signed operand found in problematic bitwise operations");
 
   bool IsUnary = false;
-  SourceLocation Location;
+  SourceLocation OperatorLoc;
 
   if (const auto *UnaryOp = N.getNodeAs<UnaryOperator>("unary-signed")) {
     IsUnary = true;
-    Location = UnaryOp->getBeginLoc();
+    OperatorLoc = UnaryOp->getOperatorLoc();
   } else {
     if (const auto *BinaryOp =
             N.getNodeAs<BinaryOperator>("binary-no-sign-interference"))
-      Location = BinaryOp->getBeginLoc();
+      OperatorLoc = BinaryOp->getOperatorLoc();
     else if (const auto *BinaryOp =
                  N.getNodeAs<BinaryOperator>("binary-sign-interference"))
-      Location = BinaryOp->getBeginLoc();
+      OperatorLoc = BinaryOp->getOperatorLoc();
     else
       llvm_unreachable("unexpected matcher result");
   }
-  diag(Location, "use of a signed integer operand with a "
+  diag(SignedOperand->getBeginLoc(), "use of a signed integer operand with a "
                                      "%select{binary|unary}0 bitwise operator")
-      << IsUnary << SignedOperand->getSourceRange();
+      << IsUnary << SignedOperand->getSourceRange() << OperatorLoc;
 }
 
 } // namespace hicpp
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/hicpp/signed-bitwise-integer-literals.cpp

 // RUN: %check_clang_tidy %s hicpp-signed-bitwise %t -- \
 // RUN:   -config="{CheckOptions: [{key: hicpp-signed-bitwise.IgnorePositiveIntegerLiterals, value: true }]}" \
 // RUN: -- -std=c++11
 
 void examples() {
   unsigned UValue = 40u;
   unsigned URes;
 
   URes = UValue & 1u; //Ok
   URes = UValue & -1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use of a signed integer operand with a binary bitwise operator
 
   unsigned URes2 = URes << 1; //Ok
 
   int IResult;
   IResult = 10 & 2; //Ok
   IResult = 3 << -1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: use of a signed integer operand with a binary bitwise operator
 
   int Int = 30;
   IResult = Int << 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
   IResult = ~0; //Ok
 }
 
 enum EnumConstruction {
   one = 1,
   two = 2,
   test1 = 1 << 12, //Ok
   test2 = one << two,
   // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use of a signed integer operand with a binary bitwise operator
   test3 = 1u << 12, //Ok
 };

clang-tools-extra/test/clang-tidy/checkers/hicpp/signed-bitwise.cpp

void binary_bitwise() {
   SResult = SValue & SValue;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
 
   UResult = SValue & 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
   UResult = SValue & -1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
   UResult&= 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
 
   UResult = UValue & 1u;     // Ok
   UResult = UValue & UValue; // Ok
   UResult&= 2u;              // Ok
 
   unsigned char UByte1 = 0u;
   unsigned char UByte2 = 16u;
   signed char SByte1 = 0;
   signed char SByte2 = 16;
 
   UByte1 = UByte1 & UByte2; // Ok
   UByte1 = SByte1 & UByte2;
   // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
   UByte1 = SByte1 & SByte2;
   // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
   SByte1 = SByte1 & SByte2;
   // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
 
   // More complex expressions.
   UResult = UValue & (SByte1 + (SByte1 | SByte2));
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: use of a signed integer operand with a binary bitwise operator
   // CHECK-MESSAGES: :[[@LINE-2]]:33: warning: use of a signed integer operand with a binary bitwise operator
 
   // The rest is to demonstrate functionality but all operators are matched equally.
   // Therefore functionality is the same for all binary operations.
   UByte1 = UByte1 | UByte2; // Ok
   UByte1 = UByte1 | SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use of a signed integer operand with a binary bitwise operator
   UByte1|= SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
   UByte1|= UByte2; // Ok
 
   UByte1 = UByte1 ^ UByte2; // Ok
   UByte1 = UByte1 ^ SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use of a signed integer operand with a binary bitwise operator
   UByte1^= SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
   UByte1^= UByte2; // Ok
 
   UByte1 = UByte1 >> UByte2; // Ok
   UByte1 = UByte1 >> SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: use of a signed integer operand with a binary bitwise operator
   UByte1>>= SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
   UByte1>>= UByte2; // Ok
 
   UByte1 = UByte1 << UByte2; // Ok
   UByte1 = UByte1 << SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: use of a signed integer operand with a binary bitwise operator
   UByte1<<= SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
   UByte1<<= UByte2; // Ok
 
   int SignedInt1 = 1 << 12;
   // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use of a signed integer operand with a binary bitwise operator
   int SignedInt2 = 1u << 12;
-  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: use of a signed integer operand with a binary bitwise operator
 }
 
 void f1(unsigned char c) {}
 void f2(signed char c) {}
 void f3(int c) {}
 
 void unary_bitwise() {
   unsigned char UByte1 = 0u;
   signed char SByte1 = 0;
 
   UByte1 = ~UByte1; // Ok
   SByte1 = ~UByte1;
   SByte1 = ~SByte1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a unary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a unary bitwise operator
   UByte1 = ~SByte1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a unary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a unary bitwise operator
 
   unsigned int UInt = 0u;
   int SInt = 0;
 
   f1(~UByte1); // Ok
   f1(~SByte1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use of a signed integer operand with a unary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use of a signed integer operand with a unary bitwise operator
   f1(~UInt);
   f1(~SInt);
-  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use of a signed integer operand with a unary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use of a signed integer operand with a unary bitwise operator
   f2(~UByte1);
   f2(~SByte1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use of a signed integer operand with a unary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use of a signed integer operand with a unary bitwise operator
   f2(~UInt);
   f2(~SInt);
-  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use of a signed integer operand with a unary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use of a signed integer operand with a unary bitwise operator
   f3(~UByte1); // Ok
   f3(~SByte1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use of a signed integer operand with a unary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use of a signed integer operand with a unary bitwise operator
 }
 
 /// HICPP uses these examples to demonstrate the rule.
 void standard_examples() {
   int i = 3;
   unsigned int k = 0u;
 
   int r = i << -1; // Emits -Wshift-count-negative from clang
   // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use of a signed integer operand with a binary bitwise operator
   r = i << 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use of a signed integer operand with a binary bitwise operator
 
   r = -1 >> -1; // Emits -Wshift-count-negative from clang
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use of a signed integer operand with a binary bitwise operator
   r = -1 >> 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use of a signed integer operand with a binary bitwise operator
 
   r = -1 >> i;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use of a signed integer operand with a binary bitwise operator
   r = -1 >> -i;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use of a signed integer operand with a binary bitwise operator
 
   r = ~0;
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use of a signed integer operand with a unary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use of a signed integer operand with a unary bitwise operator
   r = ~0u; // Ok
   k = ~k;  // Ok
 
   unsigned int u = (-1) & 2u;
   // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use of a signed integer operand with a binary bitwise operator
   u = (-1) | 1u;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use of a signed integer operand with a binary bitwise operator
   u = (-1) ^ 1u;

 enum EnumConstruction {
   one = 1,
   two = 2,
   test1 = 1 << 12,
   // CHECK-MESSAGES: [[@LINE-1]]:11: warning: use of a signed integer operand with a binary bitwise operator
   test2 = one << two,
   // CHECK-MESSAGES: [[@LINE-1]]:11: warning: use of a signed integer operand with a binary bitwise operator
   test3 = 1u << 12,
-  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: use of a signed integer operand with a binary bitwise operator
 };