[clang-tidy] hicpp bitwise operations on signed integers

[JonasToth]

Summary

This check implements the rule 5.6. HIC++
that forbidds bitwise operations on signed integer types.

[Eugene.Zelenko]

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

7

Please add short description. Copying it from ReleaseNotes.rst should be fine.

[JonasToth]

Fix documentation of checker

[JonasToth]

[Eugene.Zelenko]

clang-tidy/hicpp/SignedBitwiseCheck.cpp

59

Unnecessary empty line.

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

9

Please insert empty line above.

[JonasToth]

fix review comments from eugene

[aaron.ballman]

clang-tidy/hicpp/SignedBitwiseCheck.cpp

24

Is ignoring implicit casts the correct behavior here as far as the coding standard is concerned? Consider:

unsigned char c = 128;
f(~c); // c promotes to int before ~ is applied

Looking at the coding standard, I get the impression this is not allowed, but I'm not *super* familiar with HIC++.

43

Any particular reason this local variable is needed? If so, it should not be declared with auto.

49

s/and/an

However, I think this assert can be removed as it cannot trigger unless the AST matching facilities have broken (which will break a *lot* of tests).

51

Elide braces here and below.

53–57

I think these diagnostics should be combined into a single diagnostic using a %select. Also, you can use getSourceRange() instead of manually creating the range.

The wording of the diagnostic is a bit off, I would go with something more along the lines of: "use of a signed integer operand with a %select{binary|unary}0 bitwise operator"

58

I'm not certain there is utility in highlighting the operator, since the operand is what's at fault.

63

I don't think a note is appropriate here; I would include the SourceRange for the offending operand in the preceding diagnostic.

[JonasToth]

clang-tidy/hicpp/SignedBitwiseCheck.cpp

24

I first implemented it without ignoring the implicit integer casts, the result was, that most cases (in test cases) where not found. therefore i implemented it that way. I add an testcase for this and see how i need to adjust the matcher.

Could you help me there with the semantic, since i am not so fluent in C/C++ standardese, but i think the findings are reasonable.

43

This shortens the next 3 lines, otherwise there will be linebreaks that are bad for readability (IMHO).

[JonasToth]

adress aarons comments

[JonasToth]

adjusted diagnostics

[aaron.ballman]

clang-tidy/hicpp/SignedBitwiseCheck.cpp

24

It kind of boils down to the intention from the HIC++. Consider a test case like:

void f(int i);

void g() {
  unsigned char c = 127;
  f(~c);
}

Does f() expect to receive -128 or 128? I think this code will pass your check (ignoring the promotion means the type is unsigned char), but the actual bitwise operation is on a signed integer value because there is an integer promotion. So 127 is promoted to int, then ~ is applied, resulting in the value -128 being passed to the function.

48

You can elide the parens, or use !!SignedBinary.

52

The logic with IsBinary seems incorrect to me -- if its value is true, then it selects the "unary" option.

[JonasToth]

minor stuff and rebase

[aaron.ballman]

clang-tidy/hicpp/SignedBitwiseCheck.cpp

47

Rather than go with the negative logic, why not go with IsUnary = SignedUnary != nullptr?

[JonasToth]

clang-tidy/hicpp/SignedBitwiseCheck.cpp

24

Yeah i see, i have such cases added in the tests.
TBH. i don't know if the standard wants this covered, but the demonstrated case is definitly bad.

Would it be a good idea, to warn on assigning/initializing signed integers with unsigned integers?

The CppCoreGuidelines have some sections on that as well: Section ES.100-103

Not sure if this check should care. On the other hand, would it be nice to have a check that covers all "integer problems".

[JonasToth]

minor fix for logic, testcase improved

[aaron.ballman]

clang-tidy/hicpp/SignedBitwiseCheck.cpp

24

Yeah i see, i have such cases added in the tests.
TBH. i don't know if the standard wants this covered, but the demonstrated case is definitly bad.

I think you should ask the HIC++ people what they think; the rule text does not make it clear what the behavior should be here.

Would it be a good idea, to warn on assigning/initializing signed integers with unsigned integers?

We already have such a warning in clang (-Wsign-conversion).

The CppCoreGuidelines have some sections on that as well: Section ES.100-103

Not sure if this check should care. On the other hand, would it be nice to have a check that covers all "integer problems".

Any such check will require so many options that it is likely to be almost unusable. However, I would not be opposed to seeing a clang-tidy module that has a bunch of checks in it that relate to integer problems.

[JonasToth]

clang-tidy/hicpp/SignedBitwiseCheck.cpp

24

i think, those could land in bugprone-, and be aliases to hicpp and the cppcoreguidelines if appropriate.

depending on my time (i should have a lot of free time for 2 months)
i will try to implement some.
is there a resource with all common problems found with integers?

[JonasToth]

• get up to date with master
• added testcase for enums

[JonasToth]

• fix indendation in testcase

[JonasToth]

clang-tidy/hicpp/SignedBitwiseCheck.cpp

24

The response from Christof:

Hi Jonas,

using bitwise operators with unsigned operands is compliant with this
rule, even if the operand is promoted to signed int.

expr.unary.op in the C++ standard says "The operand of ~ shall have
integral or unscoped enumeration type; [...] Integral promotions are
performed. The type of the result is the type of the promoted operand."

The HICPP rule refers to the type of the operand (and not the type of
the promoted operand), it therefore refers to the type before promotion
(which is "unsigned char" in the example).

Regards,
Christof

Your requested case is therefore conforming and i think the current implementation handles these things correct.

[aaron.ballman]

clang-tidy/hicpp/SignedBitwiseCheck.cpp

24

Your requested case is therefore conforming and i think the current implementation handles these things correct.

Agreed; thank you for following up on this!

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

206

I don't think it helps to have this code remain in the test case.

Can you add tests for the following?

int i1 = 1 << 12;
int i2 = 1u << 12;

enum E {
  one = 1,
  two = 2,
  test1 = 1 << 12,
  test2 = one << two,
  test3 = 1u << 12
};

[JonasToth]

• added additional testcases, like @aaron.ballman requested
• fixed diagnostics

[JonasToth]

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

206

the first two cases int i1 ... are in the first function of this file( binary_bitwise).

[aaron.ballman]

Thanks! LGTM!

clang-tidy/hicpp/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyHICPPModule
   ExceptionBaseclassCheck.cpp
   NoAssemblerCheck.cpp
   HICPPTidyModule.cpp
+  SignedBitwiseCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyCppCoreGuidelinesModule
   clangTidyGoogleModule
   clangTidyMiscModule
   clangTidyModernizeModule
   clangTidyReadabilityModule
   clangTidyUtils
   )

clang-tidy/hicpp/HICPPTidyModule.cpp

 #include "../modernize/UseEqualsDefaultCheck.h"
 #include "../modernize/UseEqualsDeleteCheck.h"
 #include "../modernize/UseOverrideCheck.h"
 #include "../readability/BracesAroundStatementsCheck.h"
 #include "../readability/FunctionSizeCheck.h"
 #include "../readability/IdentifierNamingCheck.h"
 #include "ExceptionBaseclassCheck.h"
 #include "NoAssemblerCheck.h"
+#include "SignedBitwiseCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace hicpp {
 
 class HICPPModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<readability::BracesAroundStatementsCheck>(
         "hicpp-braces-around-statements");
     CheckFactories.registerCheck<ExceptionBaseclassCheck>(
         "hicpp-exception-baseclass");
+    CheckFactories.registerCheck<SignedBitwiseCheck>(
+        "hicpp-signed-bitwise");
     CheckFactories.registerCheck<google::ExplicitConstructorCheck>(
         "hicpp-explicit-conversions");
     CheckFactories.registerCheck<readability::FunctionSizeCheck>(
         "hicpp-function-size");
     CheckFactories.registerCheck<readability::IdentifierNamingCheck>(
         "hicpp-named-parameter");
     CheckFactories.registerCheck<misc::UseAfterMoveCheck>(
         "hicpp-invalid-access-moved");

clang-tidy/hicpp/SignedBitwiseCheck.h

+//===--- SignedBitwiseCheck.h - clang-tidy-----------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_HICPP_SIGNED_BITWISE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_HICPP_SIGNED_BITWISE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace hicpp {
+
+/// 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) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace hicpp
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_HICPP_SIGNED_BITWISE_H

clang-tidy/hicpp/SignedBitwiseCheck.cpp

+//===--- SignedBitwiseCheck.cpp - clang-tidy-------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "SignedBitwiseCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+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");
+
+  // Match binary bitwise operations on signed integer arguments.
+  Finder->addMatcher(
+      binaryOperator(allOf(anyOf(hasOperatorName("|"), hasOperatorName("&"),
+                                 hasOperatorName("^"), hasOperatorName("<<"),
+                                 hasOperatorName(">>")),
+                           hasEitherOperand(SignedIntegerOperand)))
+          .bind("binary_signed"),
+      this);
+
+  // Match unary operations on signed integer types.
+  Finder->addMatcher(unaryOperator(allOf(hasOperatorName("~"),
+                                         hasUnaryOperand(SignedIntegerOperand)))
+                         .bind("unary_signed"),
+                     this);
+}
+
+void SignedBitwiseCheck::check(const MatchFinder::MatchResult &Result) {
+  const ast_matchers::BoundNodes &N = Result.Nodes;
+  const auto *SignedBinary = N.getNodeAs<BinaryOperator>("binary_signed");
+  const auto *SignedUnary = N.getNodeAs<UnaryOperator>("unary_signed");
+  const auto *SignedOperand = N.getNodeAs<Expr>("signed_operand");
+
+  const bool IsNotBinary = SignedBinary == nullptr;
+  diag(IsNotBinary ? SignedUnary->getLocStart() : SignedBinary->getLocStart(),
+       "use of a signed integer operand with a %select{binary|unary}0 bitwise "
+       "operator")
+      << IsNotBinary << SignedOperand->getSourceRange();
+}
+
+} // namespace hicpp
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

- New `bugprone-integer-division
   cause unintended loss of precision.
 
 - New `hicpp-exception-baseclass
   <http://clang.llvm.org/extra/clang-tidy/checks/hicpp-exception-baseclass.html>`_ check
 
   Ensures that all exception will be instances of ``std::exception`` and classes 
   that are derived from it.
 
+- New `hicpp-signed-bitwise
+  <http://clang.llvm.org/extra/clang-tidy/checks/hicpp-signed-bitwise.html>`_ check
+
+  Finds uses of bitwise operations on signed integer types, which may lead to 
+  undefined or implementation defined behaviour.
+
 - New `readability-static-accessed-through-instance
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-static-accessed-through-instance.html>`_ check
 
   Finds member expressions that access static members through instances and
   replaces them with uses of the appropriate qualified-id.
 
 - Added `modernize-use-emplace.IgnoreImplicitConstructors
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-emplace.html#cmdoption-arg-IgnoreImplicitConstructors>`_

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/>`_.

docs/clang-tidy/checks/list.rst

.. toctree::
    hicpp-explicit-conversions (redirects to google-explicit-constructor) <hicpp-explicit-conversions>
    hicpp-function-size (redirects to readability-function-size) <hicpp-function-size>
    hicpp-invalid-access-moved (redirects to misc-use-after-move) <hicpp-invalid-access-moved>
    hicpp-member-init (redirects to cppcoreguidelines-pro-type-member-init) <hicpp-member-init>
    hicpp-named-parameter (redirects to readability-named-parameter) <hicpp-named-parameter>
    hicpp-new-delete-operators (redirects to misc-new-delete-overloads) <hicpp-new-delete-operators>
    hicpp-no-assembler
    hicpp-noexcept-move (redirects to misc-noexcept-moveconstructor) <hicpp-noexcept-move>
+   hicpp-signed-bitwise
    hicpp-special-member-functions (redirects to cppcoreguidelines-special-member-functions) <hicpp-special-member-functions>
    hicpp-undelegated-constructor (redirects to misc-undelegated-constructor) <hicpp-undelegated-constructor>
    hicpp-use-equals-default (redirects to modernize-use-equals-default) <hicpp-use-equals-default>
    hicpp-use-equals-delete (redirects to modernize-use-equals-delete) <hicpp-use-equals-delete>
    hicpp-use-override (redirects to modernize-use-override) <hicpp-use-override>
    llvm-header-guard
    llvm-include-order
    llvm-namespace-comment

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

+// RUN: %check_clang_tidy %s hicpp-signed-bitwise %t
+
+// These could cause false positives and should not be considered.
+struct StreamClass {
+};
+StreamClass &operator<<(StreamClass &os, unsigned int i) {
+  return os;
+}
+StreamClass &operator<<(StreamClass &os, int i) {
+  return os;
+}
+StreamClass &operator>>(StreamClass &os, unsigned int i) {
+  return os;
+}
+StreamClass &operator>>(StreamClass &os, int i) {
+  return os;
+}
+struct AnotherStream {
+  AnotherStream &operator<<(unsigned char c) { return *this; }
+  AnotherStream &operator<<(char c) { return *this; }
+
+  AnotherStream &operator>>(unsigned char c) { return *this; }
+  AnotherStream &operator>>(char c) { return *this; }
+};
+
+void f1(unsigned char c) {}
+void f2(char c) {}
+void f3(int c) {}
+
+void binary_bitwise() {
+  int SValue = 42;
+  int SResult;
+
+  unsigned int UValue = 42;
+  unsigned int UResult;
+
+  SResult = SValue & 1; // Bad, one operand signed and result is signed, maybe fix with suffix
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  SResult = SValue & -1; // Bad, both are signed and result is signed
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  SResult = SValue & SValue; // Bad, both are sigend and result is signed
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+
+  UResult = SValue & 1; // Bad, operation on signed, maybe fix with suffix
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = SValue & -1; // Bad, operations are signed, and even negative
+  // 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
+
+  unsigned char UByte1 = 0u;
+  unsigned char UByte2 = 16u;
+  char SByte1 = 0;
+  char SByte2 = 16;
+
+  UByte1 = UByte1 & UByte2; // Ok
+  UByte1 = SByte1 & UByte2; // Bad, one is signed
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+  UByte1 = SByte1 & SByte2; // Bad, both are signed
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+  SByte1 = SByte1 & SByte2; // Bad, both are signed and result is signed
+  // 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)); // Bad, RHS is signed
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: 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; // Bad
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+
+  UByte1 = UByte1 ^ UByte2; // Ok
+  UByte1 = UByte1 ^ SByte2; // Bad
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+
+  UByte1 = UByte1 >> UByte2; // Ok
+  UByte1 = UByte1 >> SByte2; // Bad
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+
+  UByte1 = UByte1 << UByte2; // Ok
+  UByte1 = UByte1 << SByte2; // Bad
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+}
+
+void unary_bitwise() {
+  unsigned char UByte1 = 0u;
+  char SByte1 = 0;
+
+  UByte1 = ~UByte1; // Ok
+  SByte1 = ~UByte1; // Bad?
+  SByte1 = ~SByte1; // Bad
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a unary bitwise operator
+  UByte1 = ~SByte1; // Bad
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a unary bitwise operator
+
+  unsigned int UInt = 0u;
+  int SInt = 0;
+
+  f1(~UByte1); // Ok
+  f1(~SByte1); // Bad
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use of a signed integer operand with a unary bitwise operator
+  f1(~UInt); // Bad, data loss
+  f1(~SInt); // Bad, data loss and Signed
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use of a signed integer operand with a unary bitwise operator
+  f2(~UByte1); // Bad
+  f2(~SByte1); // Bad
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use of a signed integer operand with a unary bitwise operator
+  f3(~UByte1); // Ok
+  f3(~SByte1); // Bad
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: 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;
+  // 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;
+  // 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
+  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;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use of a signed integer operand with a binary bitwise operator
+}
+
+void streams_should_work() {
+  StreamClass s;
+  s << 1u; // Ok
+  s << 1; // Ok
+  s >> 1; // Ok
+  s >> 1u; // Ok
+
+  AnotherStream as;
+  unsigned char uc = 1u;
+  char sc = 1;
+  as << uc; // Ok
+  as << sc; // Ok
+  as >> uc; // Ok
+  as >> sc; // Ok
+}