[clang-tidy] hicpp-signed-bitwise: Do not show "use of a signed integer operand with a binary bitwise operator" for positive integer operands

[vladimir.plyashkun]

Summary

Hi! Clang-Tidy has been integrated in CLion some time ago, but a lot of users started to complain on hicpp-signed-bitwise inspection as it produces a lot of noise, especially for positive integer literals.

There are already some issues (with discussions) in LLVM tracker with same problem:
https://bugs.llvm.org/show_bug.cgi?id=36961
https://bugs.llvm.org/show_bug.cgi?id=43606

In my opinion this check should be disabled in case of integer literals, since there are a lot of existing code (even in system libraries) where user can do nothing, e.g.:

#include <fcntl.h>

int main() {
    open("file", O_RDONLY | O_NOCTTY); // <-- warning here
}

[aaron.ballman]

In my opinion this check should be disabled in case of integer literals, since there are a lot of existing code (even in system libraries) where user can do nothing, e.g.:

I think that this check should behave how the coding standard dictates it behaves. By my reading of HIC++, the check is behaving by design. I think there are a few ways forward though (not mutually exclusive):

0) See if the HIC++ folks are interested in clarifying their standard for integer literals and if they are, modify the check

1. Add an off-by-default option that suppresses the diagnostic with positive integer literals so that users can explicitly decide to deviate locally
2. Document that this happens and is expected behavior; users can disable the check if they don't wish to conform to that standard.

[JonasToth]

In D68694#1701772, @aaron.ballman wrote:

In my opinion this check should be disabled in case of integer literals, since there are a lot of existing code (even in system libraries) where user can do nothing, e.g.:

I think that this check should behave how the coding standard dictates it behaves. By my reading of HIC++, the check is behaving by design. I think there are a few ways forward though (not mutually exclusive):

0) See if the HIC++ folks are interested in clarifying their standard for integer literals and if they are, modify the check

1. Add an off-by-default option that suppresses the diagnostic with positive integer literals so that users can explicitly decide to deviate locally
2. Document that this happens and is expected behavior; users can disable the check if they don't wish to conform to that standard.

Hey :)
I would personally prefer option 1. My biggest wish is to have time to come back to development in a few weeks (but not guaranteed :/) then I would iterate the check a bit to make it more userfriendly as well!

Best, Jonas

[vladimir.plyashkun]

Provide additional option to preserve current inspection behaviour

[vladimir.plyashkun]

@aaron.ballman
@JonasToth
Thanks, i agree too! I've prepared new revision with additional option to preserve old inspection behaviour.

[lebedev.ri]

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

23–24

i'm not sure this should look for a global option with such name?

[JonasToth]

The new switch needs documentation as well, and maybe even a note in the release notes (as it is publicly discussed as issue?).
Otherwise I am fine with the changes!

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

19 ↗

(On Diff #224557)

Could you please add URes << 1 as well? I believe that was problematic in the stack-overflow-question, wasn't it?

[vladimir.plyashkun]

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

23–24

I think that this method is common and used in so many inspections.
For example this check also have option called IgnoreMacros which is retrieved in same way (by calling getLocalOrGlobal method)

[lebedev.ri]

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

23–24

I'm very specifically discriminating against "IgnorePositiveIntegerLiterals" here.
I know getLocalOrGlobal() is widely used, because in those places the same flag
is used in multiple modules with same meaning.
Is that the case here?

[vladimir.plyashkun]

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

23–24

Ok, i agree. Fixed.

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

19 ↗

(On Diff #224557)

Yes, sure.

[vladimir.plyashkun]

In D68694#1705668, @JonasToth wrote:

The new switch needs documentation as well, and maybe even a note in the release notes (as it is publicly discussed as issue?).

Do you know who is responsible for it? Because i haven't worked with documentation before and don't know what i need to do to update it.

[JonasToth]

Do you know who is responsible for it? Because i haven't worked with documentation before and don't know what i need to do to update it.

The files reside in clang-tools-extra/docs/ReleaseNotes.rst and clang-tools-extra/docs/clang-tidy/checks/hicpp-signed-bitwise.rst.

You can check other files for reference, e.g. readability-magic-numbers.rst.
In the Release Notes you can add one entry at the end of the clang-tidy section.

[vladimir.plyashkun]

In D68694#1706204, @JonasToth wrote:

Do you know who is responsible for it? Because i haven't worked with documentation before and don't know what i need to do to update it.

The files reside in clang-tools-extra/docs/ReleaseNotes.rst and clang-tools-extra/docs/clang-tidy/checks/hicpp-signed-bitwise.rst.

You can check other files for reference, e.g. readability-magic-numbers.rst.
In the Release Notes you can add one entry at the end of the clang-tidy section.

Thanks! I've fixed documentation.

[JonasToth]

LGTM.
But i was inactive for a long time, if @aaron.ballman accepts as well you can commit instantly. Otherwise please let the other people that commented some time to react. :)

[aaron.ballman]

LGTM aside from a small nit.

clang-tools-extra/docs/ReleaseNotes.rst

174 ↗

(On Diff #224976)

IgnorePositiveIntegerLiterals should use two backticks instead of one.

[vladimir.plyashkun]

In D68694#1711035, @aaron.ballman wrote:

LGTM aside from a small nit.

Thanks, fixed it!

[aaron.ballman]

Thank you for the patch! I've committed on your behalf in 4de6b1586807285e20a5db6596519c2336a64568.

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

 using namespace clang::ast_matchers::internal;
 
 namespace clang {
 namespace tidy {
 namespace hicpp {
 
 void SignedBitwiseCheck::registerMatchers(MatchFinder *Finder) {
   const auto SignedIntegerOperand =
-      expr(ignoringImpCasts(hasType(isSignedInteger()))).bind("signed-operand");
+      expr(ignoringImpCasts(hasType(isSignedInteger())),
+           unless(integerLiteral()))
+          .bind("signed-operand");
 
   // The standard [bitmask.types] allows some integral types to be implemented
   // as signed types. Exclude these types from diagnosing for bitwise or(|) and
   // bitwise and(&). Shifting and complementing such values is still not
   // allowed.
   const auto BitmaskType = namedDecl(
       hasAnyName("::std::locale::category", "::std::ctype_base::mask",
                  "::std::ios_base::fmtflags", "::std::ios_base::iostate",

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

void binary_bitwise() {
 
   UByte1 = UByte1 << UByte2; // Ok
   UByte1 = UByte1 << SByte2;
   // CHECK-MESSAGES: :[[@LINE-1]]:12: 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
   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
+  int SignedInt1 = 1 << 12; // Ok
+  int SignedInt2 = 1u << 12; // Ok
 }
 
 void f1(unsigned char c) {}
 void f2(signed char c) {}
 void f3(int c) {}
 
 void unary_bitwise() {
   unsigned char UByte1 = 0u;
void standard_examples() {
   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
+  r = ~0; // Ok
   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;
void classicEnums() {
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: use of a signed integer operand with a binary bitwise operator
   s3 = s1 ^ s2; // Signed
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: use of a signed integer operand with a binary bitwise operator
 }
 
 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
+  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,
-  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: use of a signed integer operand with a binary bitwise operator
+  test3 = 1u << 12, // Ok
 };