[clang-tidy] Fix bug 34747, streaming operators and hicpp-signed-bitwise

[JonasToth]

Summary

The bug happened with stream operations, that were not recognized in all cases.
Even there were already existing test for streaming classes, they did not catch this bug.
Adding the isolated example to the existing tests did not trigger the bug.
Therefore i created a new isolated file that did expose the bug indeed.

[aaron.ballman]

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

21–27

Instead of re-testing the same behavior as another test, you can use a different trick to make lit happy: add | count 0 to the end of the RUN line. We also put in a comment like:

// Note: this test expects no diagnostics, but FileCheck cannot handle that,
// hence the use of | count 0.

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

1 ↗

(On Diff #117122)

Why is the target no longer needed for this test?

[JonasToth]

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

21–27

Ok. I will do this.

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

1 ↗

(On Diff #117122)

That line was introduced by chapuni to silence the buildbot.

The original error came from ARM having char being unsigned. I changed all occurence of char to unsigned char or signed char in my fix patch, making specifying the target unnecessary.

[JonasToth]

• silence lit the good way

[aaron.ballman]

LGTM, with a comment about the unrelated test change.

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

1 ↗

(On Diff #117122)

Thanks for the explanation. It sounds like that's unrelated to the current functionality fix, so it should be a separate commit. No review needed for that one.

[JonasToth]

• add the target declaration for test case back

[malcolm.parsons]

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

15

Typo: occured -> occurred.

[JonasToth]

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

15

Fixed in the next bugfix for standard enums, that are allowed to be signed.

clang-tidy/hicpp/SignedBitwiseCheck.cpp

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)))
+                           hasEitherOperand(SignedIntegerOperand),
+                           hasLHS(hasType(isInteger())),
+                           hasRHS(hasType(isInteger()))))
           .bind("binary_signed"),
       this);
 
   // Match unary operations on signed integer types.
   Finder->addMatcher(unaryOperator(allOf(hasOperatorName("~"),
                                          hasUnaryOperand(SignedIntegerOperand)))
                          .bind("unary_signed"),
                      this);

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

+// RUN: %check_clang_tidy %s hicpp-signed-bitwise %t -- -- -std=c++11 | count 0
+
+// Note: this test expects no diagnostics, but FileCheck cannot handle that,
+// hence the use of | count 0.
+
+template <typename C>
+struct OutputStream {
+  OutputStream &operator<<(C);
+};
+
+template <typename C>
+struct foo {
+  typedef OutputStream<C> stream_type;
+  foo(stream_type &o) {
+    o << 'x'; // warning occured here, fixed now
+  }
+};
+
+void bar(OutputStream<signed char> &o) {
+  foo<signed char> f(o);
+}