[PATCH] D128372: [Clang-Tidy] Empty Check

Nathan James via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Aug 5 23:56:12 PDT 2022

njames93 added inline comments.

Comment at: clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp:124
+          return isa<CXXMethodDecl>(ND) &&
+                 llvm::dyn_cast<CXXMethodDecl>(ND)->getMinRequiredArguments() ==
+                     0;
`dyn_cast` isn't needed here as we have already checked we are dealing with a `CXXMethodDecl`.

Comment at: clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp:132
+      diag(MemberLoc,
+           "ignoring the result of 'empty()', did you mean 'clear()'? ")
+          << FixItHint::CreateReplacement(ReplacementRange, "clear");
The 'did you mean' diagnostics in clang all use a semicolon `;` instead of a comma between the message and suggestion.

Comment at: clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp:177-178
+      diag(NonMemberLoc, "ignoring the result of '%0', did you mean 'clear()'?")
+          << llvm::dyn_cast<NamedDecl>(NonMemberCall->getCalleeDecl())
+                 ->getQualifiedNameAsString()
+          << FixItHint::CreateReplacement(ReplacementRange, ReplacementText);
Diagnostics can accept args as `const NamedDecl *`, so you can omit the call to `getQualifiedNameAsString()`.
The use of `dyn_cast` here is suspicious. If the CalleeDecl isn't a `NamedDecl`, this would assert when you try and call `getQualifiedNameAsString`, but equally I can't see any case why the CalleeDecl wouldn't be a `NamedDecl`. @rsmith Can you think of any situation where this could happen?

Comment at: clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp:57-72
+  using ast_matchers::callee;
+  using ast_matchers::callExpr;
+  using ast_matchers::cxxMemberCallExpr;
+  using ast_matchers::cxxMethodDecl;
+  using ast_matchers::expr;
+  using ast_matchers::functionDecl;
+  using ast_matchers::hasName;
These using declarations are annoying, the common convention in tidy checks is to just use a global `using namespace ast_matchers` at the top of the file.

Comment at: clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp:120-121
+        llvm::find_if(Methods, [](const CXXMethodDecl *F) {
+          return F->getDeclName().getAsString() == "clear" &&
+                 F->getMinRequiredArguments() == 0;
+        });
We also need to check qualifiers. If there exists a clear method but its unavailable due to it not being const when our member is const. The fix would result in a compile error.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list