[clang-tidy] Fix bug 34845, offending standard bitmask types

[JonasToth]

Summary

The C++ standard allows implementations to choose the underlying type for
bitmask types (e.g. std::ios_base::openmode). MSVC implemented some of them
as signed integers resulting in warnings for usual code like
auto dd = std::ios_base::badbit | std::ios_base::failbit;

These false positives were reported in https://bugs.llvm.org/show_bug.cgi?id=34845

The fix allows bitwise |,&,^ for known standard bitmask types under the condition
that both operands are such bitmask types.
Shifting and bitwise complement are still forbidden.

[JonasToth]

• fix run line for unit test, unwanted change

[JonasToth]

• remove debug output

[aaron.ballman]

clang-tidy/hicpp/SignedBitwiseCheck.cpp

78 ↗

(On Diff #119596)

No need to re-initialize the value here. You can drop it here or remove the initialization above.

82 ↗

(On Diff #119596)

Drop the newline.

86 ↗

(On Diff #119596)

Drop the newline.

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

6 ↗

(On Diff #119596)

because the appear -> because they appear

I think a better way to handle this might be to put the enumerations into a local header file that uses #pragma clang system_header, and then #include that file to get the declarations in the test.

[JonasToth]

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

6 ↗

(On Diff #119596)

Typo is fixed. I copy&pasted the definitions into a header file like you suggested, but then 'check_clang_tidy' is not happy. It does not seem to find the included header.

I believe, this is due to the path from where it is executed? Having it run manually works though.
Maybe I could just leave them here?

[aaron.ballman]

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

6 ↗

(On Diff #119596)

I would strongly prefer not to, as we also want to test that these warnings do not appear in system headers. I am not certain why check_clang_tidy is unable to find the header file, especially if the header is next to the source file. That would be worth tracking down as a deficiency with check_clang_tidy.

[JonasToth]

• address review comments
• fix run line, check-clang-tidy will not be invoked

[JonasToth]

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

6 ↗

(On Diff #119596)

What works is using 'clang-tidy' directly in the RUN line.

I tried a little bit to figure out why clang-tidy-check does not do it correctly, but i currently have not enough time.
Maybe i can investigate it later this weekend or next week.

[JonasToth]

ping :)

[aaron.ballman]

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

3 ↗

(On Diff #119678)

Why does this include need to go up a directory level? The header file resides next to the source file, so I would expect this to not find the include.

test/clang-tidy/hicpp-signed-bitwise-standard-types.h

82 ↗

(On Diff #119678)

Extra newline can be removed.

[JonasToth]

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

3 ↗

(On Diff #119678)

Inspecting the testoutput, check-clang-tidy does create its own directory from where its run. I tried both version (with '../' and without) and both are functional. I can remove if you want.

[JonasToth]

• adjust run line and include
• remove whitespace

[JonasToth]

• remove conflict markers

[aaron.ballman]

LGTM!

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

3 ↗

(On Diff #119678)

It's less confusing with it removed.

clang-tools-extra/trunk/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()))).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(anyOf(
+      hasName("::std::locale::category"), hasName("::std::ctype_base::mask"),
+      hasName("::std::ios_base::fmtflags"), hasName("::std::ios_base::iostate"),
+      hasName("::std::ios_base::openmode")));
+  const auto IsStdBitmask = ignoringImpCasts(declRefExpr(hasType(BitmaskType)));
 
   // Match binary bitwise operations on signed integer arguments.
   Finder->addMatcher(
-      binaryOperator(allOf(anyOf(hasOperatorName("|"), hasOperatorName("&"),
-                                 hasOperatorName("^"), hasOperatorName("<<"),
-                                 hasOperatorName(">>")),
+      binaryOperator(
+          allOf(anyOf(hasOperatorName("^"), hasOperatorName("|"),
+                      hasOperatorName("&")),
+
+                unless(allOf(hasLHS(IsStdBitmask), hasRHS(IsStdBitmask))),
+
+                hasEitherOperand(SignedIntegerOperand),
+                hasLHS(hasType(isInteger())), hasRHS(hasType(isInteger()))))
+          .bind("binary-no-sign-interference"),
+      this);
+
+  // Shifting and complement is not allowed for any signed integer type because
+  // the sign bit may corrupt the result.
+  Finder->addMatcher(
+      binaryOperator(allOf(anyOf(hasOperatorName("<<"), hasOperatorName(">>")),
                            hasEitherOperand(SignedIntegerOperand),
                            hasLHS(hasType(isInteger())),
                            hasRHS(hasType(isInteger()))))
-          .bind("binary_signed"),
+          .bind("binary-sign-interference"),
       this);
 
   // Match unary operations on signed integer types.
   Finder->addMatcher(unaryOperator(allOf(hasOperatorName("~"),
                                          hasUnaryOperand(SignedIntegerOperand)))
-                         .bind("unary_signed"),
+                         .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 auto *SignedOperand = N.getNodeAs<Expr>("signed-operand");
+  assert(SignedOperand &&
+         "No signed operand found in problematic bitwise operations");
+
+  bool IsUnary = false;
+  SourceLocation Location;
+
+  if (const auto *UnaryOp = N.getNodeAs<UnaryOperator>("unary-signed")) {
+    IsUnary = true;
+    Location = UnaryOp->getLocStart();
+  } else {
+    if (const auto *BinaryOp =
+            N.getNodeAs<BinaryOperator>("binary-no-sign-interference"))
+      Location = BinaryOp->getLocStart();
+    else if (const auto *BinaryOp =
+                 N.getNodeAs<BinaryOperator>("binary-sign-interference"))
+      Location = BinaryOp->getLocStart();
+    else
+      llvm_unreachable("unexpected matcher result");
+  }
 
-  const bool IsUnary = SignedUnary != nullptr;
-  diag(IsUnary ? SignedUnary->getLocStart() : SignedBinary->getLocStart(),
+  diag(Location,
        "use of a signed integer operand with a %select{binary|unary}0 bitwise "
        "operator")
       << IsUnary << SignedOperand->getSourceRange();
 }
 
 } // namespace hicpp
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/test/clang-tidy/hicpp-signed-bitwise-standard-types.h

+#pragma clang system_header
+
+// Implement standard types that are known to be defined as unsigned in some
+// implementations like MSVC.
+namespace std {
+namespace locale {
+enum category : int {
+  none = 0u,
+  collate = 1u << 1u,
+  ctype = 1u << 2u,
+  monetary = 1u << 3u,
+  numeric = 1u << 4u,
+  time = 1u << 5u,
+  messages = 1u << 6u,
+  all = none | collate | ctype | monetary | numeric | time | messages
+  // CHECK MESSAGES: [[@LINE-1]]:9: warning: use of a signed integer operand with a binary bitwise operator
+};
+} // namespace locale
+
+namespace ctype_base {
+enum mask : int {
+  space,
+  print,
+  cntrl,
+  upper,
+  lower,
+  alpha,
+  digit,
+  punct,
+  xdigit,
+  /* blank, // C++11 */
+  alnum = alpha | digit,
+  // CHECK MESSAGES: [[@LINE-1]]:11: warning: use of a signed integer operand with a binary bitwise operator
+  graph = alnum | punct
+  // CHECK MESSAGES: [[@LINE-1]]:11: warning: use of a signed integer operand with a binary bitwise operator
+};
+} // namespace ctype_base
+
+namespace ios_base {
+enum fmtflags : int {
+  dec = 0u,
+  oct = 1u << 2u,
+  hex = 1u << 3u,
+  basefield = dec | oct | hex | 0u,
+  // CHECK MESSAGES: [[@LINE-1]]:15: warning: use of a signed integer operand with a binary bitwise operator
+  left = 1u << 4u,
+  right = 1u << 5u,
+  internal = 1u << 6u,
+  adjustfield = left | right | internal,
+  // CHECK MESSAGES: [[@LINE-1]]:17: warning: use of a signed integer operand with a binary bitwise operator
+  scientific = 1u << 7u,
+  fixed = 1u << 8u,
+  floatfield = scientific | fixed | (scientific | fixed) | 0u,
+  // CHECK MESSAGES: [[@LINE-1]]:16: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK MESSAGES: [[@LINE-2]]:38: warning: use of a signed integer operand with a binary bitwise operator
+  boolalpha = 1u << 9u,
+  showbase = 1u << 10u,
+  showpoint = 1u << 11u,
+  showpos = 1u << 12u,
+  skipws = 1u << 13u,
+  unitbuf = 1u << 14u,
+  uppercase = 1u << 15u
+};
+
+enum iostate : int {
+  goodbit = 0u,
+  badbit = 1u << 1u,
+  failbit = 1u << 2u,
+  eofbit = 1u << 3u
+};
+
+enum openmode : int {
+  app = 0u,
+  binary = 0u << 1u,
+  in = 0u << 2u,
+  out = 0u << 3u,
+  trunc = 0u << 4u,
+  ate = 0u << 5u
+};
+} // namespace ios_base
+} // namespace std

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

+// RUN: clang-tidy %s -checks='-*,hicpp-signed-bitwise' -- -std=c++11
+
+#include "hicpp-signed-bitwise-standard-types.h"
+
+void pure_bitmask_types() {
+  // std::locale::category
+  int SResult = 0;
+  std::locale::category C = std::locale::category::ctype;
+
+  SResult = std::locale::category::none | std::locale::category::collate;
+  SResult = std::locale::category::ctype & std::locale::category::monetary;
+  SResult = std::locale::category::numeric ^ std::locale::category::time;
+  SResult = std::locale::category::messages | std::locale::category::all;
+
+  SResult = std::locale::category::all & C;
+  SResult = std::locale::category::all | C;
+  SResult = std::locale::category::all ^ C;
+
+  // std::ctype_base::mask
+  std::ctype_base::mask M = std::ctype_base::mask::punct;
+
+  SResult = std::ctype_base::mask::space | std::ctype_base::mask::print;
+  SResult = std::ctype_base::mask::cntrl & std::ctype_base::mask::upper;
+  SResult = std::ctype_base::mask::lower ^ std::ctype_base::mask::alpha;
+  SResult = std::ctype_base::mask::digit | std::ctype_base::mask::punct;
+  SResult = std::ctype_base::mask::xdigit & std::ctype_base::mask::alnum;
+  SResult = std::ctype_base::mask::alnum ^ std::ctype_base::mask::graph;
+
+  SResult = std::ctype_base::mask::space & M;
+  SResult = std::ctype_base::mask::space | M;
+  SResult = std::ctype_base::mask::space ^ M;
+
+  // std::ios_base::fmtflags
+  std::ios_base::fmtflags F = std::ios_base::fmtflags::floatfield;
+
+  SResult = std::ios_base::fmtflags::dec | std::ios_base::fmtflags::oct;
+  SResult = std::ios_base::fmtflags::hex & std::ios_base::fmtflags::basefield;
+  SResult = std::ios_base::fmtflags::left ^ std::ios_base::fmtflags::right;
+  SResult = std::ios_base::fmtflags::internal | std::ios_base::fmtflags::adjustfield;
+  SResult = std::ios_base::fmtflags::scientific & std::ios_base::fmtflags::fixed;
+  SResult = std::ios_base::fmtflags::floatfield ^ std::ios_base::fmtflags::boolalpha;
+  SResult = std::ios_base::fmtflags::showbase | std::ios_base::fmtflags::showpoint;
+  SResult = std::ios_base::fmtflags::showpos & std::ios_base::fmtflags::skipws;
+  SResult = std::ios_base::fmtflags::unitbuf ^ std::ios_base::fmtflags::uppercase;
+
+  SResult = std::ios_base::fmtflags::unitbuf | F;
+  SResult = std::ios_base::fmtflags::unitbuf & F;
+  SResult = std::ios_base::fmtflags::unitbuf ^ F;
+
+  // std::ios_base::iostate
+  std::ios_base::iostate S = std::ios_base::iostate::goodbit;
+
+  SResult = std::ios_base::iostate::goodbit | std::ios_base::iostate::badbit;
+  SResult = std::ios_base::iostate::failbit & std::ios_base::iostate::eofbit;
+  SResult = std::ios_base::iostate::failbit ^ std::ios_base::iostate::eofbit;
+
+  SResult = std::ios_base::iostate::goodbit | S;
+  SResult = std::ios_base::iostate::goodbit & S;
+  SResult = std::ios_base::iostate::goodbit ^ S;
+
+  // std::ios_base::openmode
+  std::ios_base::openmode B = std::ios_base::openmode::binary;
+
+  SResult = std::ios_base::openmode::app | std::ios_base::openmode::binary;
+  SResult = std::ios_base::openmode::in & std::ios_base::openmode::out;
+  SResult = std::ios_base::openmode::trunc ^ std::ios_base::openmode::ate;
+
+  SResult = std::ios_base::openmode::trunc | B;
+  SResult = std::ios_base::openmode::trunc & B;
+  SResult = std::ios_base::openmode::trunc ^ B;
+}
+
+void still_forbidden() {
+  // std::locale::category
+  unsigned int UResult = 0u;
+  int SResult = 0;
+
+  SResult = std::ctype_base::mask::print ^ 8u;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  SResult = std::ctype_base::mask::cntrl | 8;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  SResult = std::ctype_base::mask::upper & 8;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  SResult = std::ctype_base::mask::lower ^ -8;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+
+  UResult = std::locale::category::collate << 1u;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::locale::category::ctype << 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::locale::category::monetary >> 1u;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::locale::category::numeric >> 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+
+  UResult = ~std::locale::category::messages;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a unary bitwise operator
+  SResult = ~std::locale::category::all;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a unary bitwise operator
+
+  // std::ctype_base::mask
+  UResult = std::ctype_base::mask::space | 8;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ctype_base::mask::print & 8u;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ctype_base::mask::cntrl ^ -8;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+
+  UResult = std::ctype_base::mask::upper << 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ctype_base::mask::lower << 1u;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ctype_base::mask::alpha >> 1u;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ctype_base::mask::digit >> 1u;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+
+  UResult = ~std::ctype_base::mask::punct;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a unary bitwise operator
+  SResult = ~std::ctype_base::mask::xdigit;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a unary bitwise operator
+
+  // std::ios_base::fmtflags
+  UResult = std::ios_base::fmtflags::dec | 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ios_base::fmtflags::oct & 1u;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ios_base::fmtflags::hex ^ -1;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+
+  UResult = std::ios_base::fmtflags::basefield >> 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ios_base::fmtflags::left >> 1u;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ios_base::fmtflags::right << 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ios_base::fmtflags::internal << 1u;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+
+  UResult = ~std::ios_base::fmtflags::adjustfield;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a unary bitwise operator
+  SResult = ~std::ios_base::fmtflags::scientific;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a unary bitwise operator
+
+  // std::ios_base::iostate
+  UResult = std::ios_base::iostate::goodbit | 8;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ios_base::iostate::badbit & 8u;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ios_base::iostate::failbit ^ -8;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+
+  UResult = std::ios_base::iostate::eofbit << 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ios_base::iostate::goodbit << 1u;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ios_base::iostate::badbit >> 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ios_base::iostate::failbit >> 1u;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+
+  UResult = ~std::ios_base::iostate::eofbit;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a unary bitwise operator
+  SResult = ~std::ios_base::iostate::goodbit;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a unary bitwise operator
+
+  // std::ios_base::openmode
+  UResult = std::ios_base::app | 8;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ios_base::binary & 8u;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ios_base::in ^ -8;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+
+  UResult = std::ios_base::out >> 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ios_base::trunc >> 1u;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ios_base::ate << 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  UResult = std::ios_base::ate << 1u;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+
+  UResult = ~std::ios_base::openmode::app;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a unary bitwise operator
+  SResult = ~std::ios_base::openmode::binary;
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use of a signed integer operand with a unary bitwise operator
+}