[clang-tidy] readability-container-size-empty: simplify implementation

[steveire]

Summary

Use IgnoreUnlessSpelledInSource to make the matcher code smaller and
more visibly-related to the code.

[alexfh]

Pre-merge builder can't apply this patch: https://buildkite.com/llvm-project/diff-checks/builds/18651
Is it based on https://reviews.llvm.org/D91302 ? Do we need the intermediate state? Maybe squash the two patches together for simplicity?

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp

127

It looks like the changes drop the requirement that either operand is 0 or 1 -- is that an oversight or can you explain why that's correct?

169

We seem to lose this case entirely?

[steveire]

clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp

127

Sorry, that was an old/intermediate version of the patch.

169

In IgnoreUnlessSpelledInSource mode, ignoringImpCasts(cxxBindTemporaryExpr(has(DefaultConstructor))) simplifies to cxxConstructExpr(isDefaultConstruction()), which is there.

[steveire]

Update

[steveire]

Update

[aaron.ballman]

LGTM!

clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp

169

Ah, exactly right, thanks!

[alexfh]

LG

[alexfh]

clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp

480–482

Wait, is losing these diagnostics necessary?

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp

480–482

I think it's an improvement. The noexcept-ness of size() need not relate to the noexcept-ness of empty() (notionally it should, but it's not a requirement users must follow), so there's no guarantee that the change will have the same semantic effect on the declaration of func().

clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h

 class ContainerSizeEmptyCheck : public ClangTidyCheck {
 public:
   ContainerSizeEmptyCheck(StringRef Name, ClangTidyContext *Context);
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  llvm::Optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONTAINERSIZEEMPTYCHECK_H

clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp

auto Result =
                      binaryOperator(hasAnyOperatorName("&&", "||")),
                      unaryOperator(hasOperatorName("!")).bind("NegOnSize"))))))
           .matches(Node, Finder, Builder);
   Builder->removeBindings([ExprName](const BoundNodesMap &Nodes) {
     return Nodes.getNode(ExprName).getNodeKind().isNone();
   });
   return Result;
 }
+AST_MATCHER(CXXConstructExpr, isDefaultConstruction) {
+  return Node.getConstructor()->isDefaultConstructor();
+}
 } // namespace ast_matchers
 namespace tidy {
 namespace readability {
 
 using utils::IsBinaryOrTernary;
 
 ContainerSizeEmptyCheck::ContainerSizeEmptyCheck(StringRef Name,
                                                  ClangTidyContext *Context)
const auto ValidContainerNonTemplateType =
           recordType(hasDeclaration(ValidContainerRecord))));
   const auto ValidContainerTemplateType =
       qualType(hasUnqualifiedDesugaredType(templateSpecializationType(
           hasDeclaration(classTemplateDecl(has(ValidContainerRecord))))));
 
   const auto ValidContainer = qualType(
       anyOf(ValidContainerNonTemplateType, ValidContainerTemplateType));
 
-  const auto WrongUse = traverse(
-      TK_AsIs,
-      anyOf(
-          hasParent(binaryOperator(isComparisonOperator(),
-                                   hasEitherOperand(ignoringImpCasts(
-                                       anyOf(integerLiteral(equals(1)),
-                                             integerLiteral(equals(0))))))
+  const auto WrongUse =
+      anyOf(hasParent(binaryOperator(
+                          isComparisonOperator(),
+                          hasEitherOperand(anyOf(integerLiteral(equals(1)),
+                                                 integerLiteral(equals(0)))))
                           .bind("SizeBinaryOp")),
-          hasParent(implicitCastExpr(
-              hasImplicitDestinationType(booleanType()),
-              anyOf(hasParent(
-                        unaryOperator(hasOperatorName("!")).bind("NegOnSize")),
-                    anything()))),
-          usedInBooleanContext()));
+            usedInBooleanContext());
 
   Finder->addMatcher(
-      cxxMemberCallExpr(unless(isInTemplateInstantiation()),
-                        on(expr(anyOf(hasType(ValidContainer),
+      cxxMemberCallExpr(on(expr(anyOf(hasType(ValidContainer),
                                       hasType(pointsTo(ValidContainer)),
                                       hasType(references(ValidContainer))))
                                .bind("MemberCallObject")),
                         callee(cxxMethodDecl(hasName("size"))), WrongUse,
                         unless(hasAncestor(cxxMethodDecl(
                             ofClass(equalsBoundNode("container"))))))
           .bind("SizeCallExpr"),
       this);
 
   Finder->addMatcher(
       callExpr(has(cxxDependentScopeMemberExpr(
                    hasObjectExpression(
                        expr(anyOf(hasType(ValidContainer),
                                   hasType(pointsTo(ValidContainer)),
                                   hasType(references(ValidContainer))))
                            .bind("MemberCallObject")),
                    hasMemberName("size"))),
                WrongUse,
                unless(hasAncestor(
                    cxxMethodDecl(ofClass(equalsBoundNode("container"))))))
           .bind("SizeCallExpr"),
       this);
 
-  // Empty constructor matcher.
-  const auto DefaultConstructor = cxxConstructExpr(
-          hasDeclaration(cxxConstructorDecl(isDefaultConstructor())));
   // Comparison to empty string or empty constructor.
   const auto WrongComparend = anyOf(
-      ignoringImpCasts(stringLiteral(hasSize(0))),
-      ignoringImpCasts(cxxBindTemporaryExpr(has(DefaultConstructor))),
-      ignoringImplicit(DefaultConstructor),
-      cxxConstructExpr(hasDeclaration(cxxConstructorDecl(isCopyConstructor())),
-                       has(expr(ignoringImpCasts(DefaultConstructor)))),
-      cxxConstructExpr(hasDeclaration(cxxConstructorDecl(isMoveConstructor())),
-                       has(expr(ignoringImpCasts(DefaultConstructor)))),
+      stringLiteral(hasSize(0)), cxxConstructExpr(isDefaultConstruction()),
       cxxUnresolvedConstructExpr(argumentCountIs(0)));
   // Match the object being compared.
   const auto STLArg =
       anyOf(unaryOperator(
                 hasOperatorName("*"),
                 hasUnaryOperand(
                     expr(hasType(pointsTo(ValidContainer))).bind("Pointee"))),
             expr(hasType(ValidContainer)).bind("STLObject"));
   Finder->addMatcher(
       cxxOperatorCallExpr(
-          unless(isInTemplateInstantiation()),
           hasAnyOverloadedOperatorName("==", "!="),
           anyOf(allOf(hasArgument(0, WrongComparend), hasArgument(1, STLArg)),
                 allOf(hasArgument(0, STLArg), hasArgument(1, WrongComparend))),
           unless(hasAncestor(
               cxxMethodDecl(ofClass(equalsBoundNode("container"))))))
           .bind("BinCmp"),
       this);
   Finder->addMatcher(

clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp

 constexpr Lazy L;
 static_assert(!L.size(), "");
 // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: the 'empty' method should be used
 // CHECK-MESSAGES: :101:18: note: method 'Lazy'::empty() defined here
 // CHECK-FIXES: {{^}}static_assert(L.empty(), "");
 
 struct StructWithLazyNoexcept {
   void func() noexcept(L.size());
-// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: the 'empty' method should be used
-// CHECK-MESSAGES: :101:18: note: method 'Lazy'::empty() defined here
-// CHECK-FIXES: {{^  }}void func() noexcept(!L.empty());
 };
 
 #define CHECKSIZE(x) if (x.size()) {}
 // CHECK-FIXES: #define CHECKSIZE(x) if (x.size()) {}
 
 template <typename T> void f() {
   std::vector<T> v;
   if (v.size())