[clang-tidy] bugprone-bool-pointer-implicit-conversion doesn't handle members

[tetsuo-cpp]

Summary

Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=45189
This patch is adding support for members to the bugprone-bool-pointer-implicit-conversion check. Apologies if I've done this in a weird/wrong way, I'm still getting my head around the matching DSL.

[tetsuo-cpp]

Remove the unnecessary template.

[njames93]

Thanks for working on this.

There seems to be many places in this check where names are confusing, Var referring to a MemberExpr, DeclRef refering to a MemberExpr. I'd prefer if the names that you have added were renamed to something more in line with what they represent.

clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp

102

This should be an else if or the previous if should return. Saves redundant checking for MemberExpr when DeclRefExpr already matched.

clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp

110–128

nit: You don't need to recycle all the test cases, the logic for that is already checked above, just a few showing member expr being detected properly will suffice.

Don't have huge preference on this so feel free to ignore.

[tetsuo-cpp]

Address comments: Rename variables, remove unnecessary tests and avoid unnecessary check.

[tetsuo-cpp]

Friendly ping!

[tetsuo-cpp]

Friendly ping. Could I please get this looked at?

[aaron.ballman]

LGTM, though I have a possible code simplification if you think it's an improvement.

clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp

78

Can you get away with:

if (const auto *E = Result.Nodes.getNodeAs<Expr>("expr")) {
  const Decl *D = isa<DeclRefExpr> ? cast<DeclRefExpr>(E)->getDecl() : cast<MemberExpr>(E)->getMemberDecl();
  const auto M = ignoringParenImpCasts(anyOf(declRefExpr(to(equalsNode(D))), memberExpr(hasDeclaration(equalsNode(D)))));
  checkImpl(Result, E, If, M, *this);
}

(Totally untested, but it hopefully demonstrates the point.)

[tetsuo-cpp]

Cleanup check conditional.

[tetsuo-cpp]

@aaron.ballman
Thanks! I took your suggestion and I think this looks cleaner.

[tetsuo-cpp]

Friendly ping @njames93.
Does this look ok to you? If so, could you please help me with the commit?

[aaron.ballman]

Re-accepting the changes.

[aaron.ballman]

Thank you for the patch, I've gone ahead and commit on your behalf in a44161692ae879068d4086a7e568a348800ba01d.

clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp

 namespace bugprone {
 
 void BoolPointerImplicitConversionCheck::registerMatchers(MatchFinder *Finder) {
   // Look for ifs that have an implicit bool* to bool conversion in the
   // condition. Filter negations.
   Finder->addMatcher(
       traverse(
           ast_type_traits::TK_AsIs,
-          ifStmt(hasCondition(findAll(implicitCastExpr(
-                     unless(hasParent(unaryOperator(hasOperatorName("!")))),
-                     hasSourceExpression(expr(
-                         hasType(pointerType(pointee(booleanType()))),
-                         ignoringParenImpCasts(declRefExpr().bind("expr")))),
-                     hasCastKind(CK_PointerToBoolean)))),
-                 unless(isInTemplateInstantiation()))
+          ifStmt(
+              hasCondition(findAll(implicitCastExpr(
+                      ignoringParenImpCasts(anyOf(declRefExpr().bind("expr"),
+                                                  memberExpr().bind("expr"))))),
               .bind("if")),
       this);
 }
 
-void BoolPointerImplicitConversionCheck::check(
-    const MatchFinder::MatchResult &Result) {
-  auto *If = Result.Nodes.getNodeAs<IfStmt>("if");
-  auto *Var = Result.Nodes.getNodeAs<DeclRefExpr>("expr");
-
+static void checkImpl(const MatchFinder::MatchResult &Result, const Expr *Ref,
+                      const IfStmt *If,
+                      const ast_matchers::internal::Matcher<Expr> &RefMatcher,
+                      ClangTidyCheck &Check) {
   // Ignore macros.
-  if (Var->getBeginLoc().isMacroID())
+  if (Ref->getBeginLoc().isMacroID())
     return;
 
-  // Only allow variable accesses for now, no function calls or member exprs.
+  // Only allow variable accesses and member exprs for now, no function calls.
   // Check that we don't dereference the variable anywhere within the if. This
   // avoids false positives for checks of the pointer for nullptr before it is
   // dereferenced. If there is a dereferencing operator on this variable don't
   // emit a diagnostic. Also ignore array subscripts.
-  const Decl *D = Var->getDecl();
-  auto DeclRef = ignoringParenImpCasts(declRefExpr(to(equalsNode(D))));
-  if (!match(findAll(
-                 unaryOperator(hasOperatorName("*"), hasUnaryOperand(DeclRef))),
+  if (!match(findAll(unaryOperator(hasOperatorName("*"),
+                                   hasUnaryOperand(RefMatcher))),
              *If, *Result.Context)
            .empty() ||
-      !match(findAll(arraySubscriptExpr(hasBase(DeclRef))), *If,
+      !match(findAll(arraySubscriptExpr(hasBase(RefMatcher))), *If,
              *Result.Context)
            .empty() ||
       // FIXME: We should still warn if the paremater is implicitly converted to
       // bool.
-      !match(findAll(callExpr(hasAnyArgument(ignoringParenImpCasts(DeclRef)))),
-             *If, *Result.Context)
+      !match(
+           findAll(callExpr(hasAnyArgument(ignoringParenImpCasts(RefMatcher)))),
            .empty() ||
-      !match(findAll(cxxDeleteExpr(has(ignoringParenImpCasts(expr(DeclRef))))),
-             *If, *Result.Context)
+      !match(
+           findAll(cxxDeleteExpr(has(ignoringParenImpCasts(expr(RefMatcher))))),
            .empty())
     return;
 
-  diag(Var->getBeginLoc(), "dubious check of 'bool *' against 'nullptr', did "
-                           "you mean to dereference it?")
-      << FixItHint::CreateInsertion(Var->getBeginLoc(), "*");
+  Check.diag(Ref->getBeginLoc(),
+             "dubious check of 'bool *' against 'nullptr', did "
+      << FixItHint::CreateInsertion(Ref->getBeginLoc(), "*");
+}
+
+void BoolPointerImplicitConversionCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *If = Result.Nodes.getNodeAs<IfStmt>("if");
+
+  if (const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>("expr")) {
+    const Decl *D = DeclRef->getDecl();
+    const auto DeclRefMatcher =
+        ignoringParenImpCasts(declRefExpr(to(equalsNode(D))));
+    checkImpl(Result, DeclRef, If, DeclRefMatcher, *this);
+  } else if (const auto *MemberRef =
+                 Result.Nodes.getNodeAs<MemberExpr>("expr")) {
+    const Decl *D = MemberRef->getMemberDecl();
+    const auto MemberRefMatcher =
+        ignoringParenImpCasts(memberExpr(hasDeclaration(equalsNode(D))));
+    checkImpl(Result, MemberRef, If, MemberRefMatcher, *this);
+  }
 }
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp

#define TESTMACRO if (b || F())
   if (c) {
     *(c) = false; // no-warning
   }
 
   struct {
     bool *b;
   } d = { SomeFunction() };
 
-  if (d.b)
+  if (d.b) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: dubious check of 'bool *' against 'nullptr'
+    // CHECK-FIXES: if (*d.b) {
+  }
+
+  if (d.b) {
     (void)*d.b; // no-warning
+  }
 
-#define CHECK(b) if (b) {}
+#define CHECK(b) \
+  if (b) {       \
+  }
   CHECK(c)
 }
+
+struct H {
+  bool *b;
+  void foo() const {
+    if (b) {
+      // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: dubious check of 'bool *' against 'nullptr'
+      // CHECK-FIXES: if (*b) {
+    }
+
+    if (b) {
+      (void)*b; // no-warning
+    }
+  }
+};