[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

20

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

20

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

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

 
 /// This check implements the rule 5.6.1 of the HICPP Standard, which disallows
 /// bitwise operations on signed integer types.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/hicpp-signed-bitwise.html
 class SignedBitwiseCheck : public ClangTidyCheck {
 public:
-  SignedBitwiseCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  SignedBitwiseCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Options) override;
+
+private:
+  bool IgnorePositiveIntegerLiterals;
 };
 
 } // namespace hicpp
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_HICPP_SIGNED_BITWISE_H

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

 
 using namespace clang::ast_matchers;
 using namespace clang::ast_matchers::internal;
 
 namespace clang {
 namespace tidy {
 namespace hicpp {
 
+SignedBitwiseCheck::SignedBitwiseCheck(StringRef Name,
+                                       ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      IgnorePositiveIntegerLiterals(
+          Options.get("IgnorePositiveIntegerLiterals", false)) {}
+
+void SignedBitwiseCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IgnorePositiveIntegerLiterals",
+                IgnorePositiveIntegerLiterals);
+}
+
 void SignedBitwiseCheck::registerMatchers(MatchFinder *Finder) {
   const auto SignedIntegerOperand =
-      expr(ignoringImpCasts(hasType(isSignedInteger()))).bind("signed-operand");
+      (IgnorePositiveIntegerLiterals
+           ? expr(ignoringImpCasts(hasType(isSignedInteger())),
+                  unless(integerLiteral()))
+           : expr(ignoringImpCasts(hasType(isSignedInteger()))))
+          .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/docs/ReleaseNotes.rst

- New :doc:`llvm-prefer-isa-or-dyn-cast-in-conditionals
   ``dyn_cast<>`` where the return value is not captured. Additionally,
   finds and replaces cases that match the pattern ``var &&
   isa<X>(var)``, where ``var`` is evaluated twice.
 
 - New :doc:`modernize-use-trailing-return-type
   <clang-tidy/checks/modernize-use-trailing-return-type>` check.
 
   Rewrites function signatures to use a trailing return type.
   
+- The :doc:`hicpp-signed-bitwise
+  <clang-tidy/checks/hicpp-signed-bitwise>` now supports `IgnorePositiveIntegerLiterals`
+  option.
+
 Improvements to include-fixer
 -----------------------------
 
 - New :doc:`openmp-exception-escape
   <clang-tidy/checks/openmp-exception-escape>` check.
 
   Analyzes OpenMP Structured Blocks and checks that no exception escapes
   out of the Structured Block it was thrown in.

clang-tools-extra/docs/clang-tidy/checks/hicpp-signed-bitwise.rst

 .. title:: clang-tidy - hicpp-signed-bitwise
 
 hicpp-signed-bitwise
 ====================
 
 Finds uses of bitwise operations on signed integer types, which may lead to 
 undefined or implementation defined behaviour.
 
 The according rule is defined in the `High Integrity C++ Standard, Section 5.6.1 <http://www.codingstandard.com/section/5-6-shift-operators/>`_.
+
+Options
+-------
+
+.. option:: IgnorePositiveIntegerLiterals
+
+   If this option is set to `true`, the check will not warn on bitwise operations with positive integer literals, e.g. `~0`, `2 << 1`, etc. 
+   Default value is `false`.

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

+// RUN: %check_clang_tidy %s hicpp-signed-bitwise %t -- \
+// RUN:   -config="{CheckOptions: [{key: hicpp-signed-bitwise.IgnorePositiveIntegerLiterals, value: 1 }]}" \
+// 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
+  
+  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
+  
+  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
+};