[clang-tidy] Ignore used special-members in modernize-use-equals-delete

Special members marked as used, or with out-of-line definition should not raise an warning now. Fixes: #33759 Reviewed By: carlosgalvezp Differential Revision: https://reviews.llvm.org/D158486
llvm · Sep 3, 2023 · 208fa9a · 208fa9a
1 parent 7203497
commit 208fa9a
Show file tree

Hide file tree

Showing 4 changed files with 74 additions and 20 deletions.
diff --git a/clang-tools-extra/clang-tidy/modernize/UseEqualsDeleteCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseEqualsDeleteCheck.cpp
@@ -15,32 +15,53 @@ using namespace clang::ast_matchers;
 
 namespace clang::tidy::modernize {
 
+namespace {
+AST_MATCHER(FunctionDecl, hasAnyDefinition) {
+  if (Node.hasBody() || Node.isPure() || Node.isDefaulted() || Node.isDeleted())
+    return true;
+
+  if (const FunctionDecl *Definition = Node.getDefinition())
+    if (Definition->hasBody() || Definition->isPure() ||
+        Definition->isDefaulted() || Definition->isDeleted())
+      return true;
+
+  return false;
+}
+
+AST_MATCHER(Decl, isUsed) { return Node.isUsed(); }
+
+AST_MATCHER(CXXMethodDecl, isSpecialFunction) {
+  if (const auto *Constructor = dyn_cast<CXXConstructorDecl>(&Node))
+    return Constructor->isDefaultConstructor() ||
+           Constructor->isCopyOrMoveConstructor();
+
+  return isa<CXXDestructorDecl>(Node) || Node.isCopyAssignmentOperator() ||
+         Node.isMoveAssignmentOperator();
+}
+} // namespace
+
 static const char SpecialFunction[] = "SpecialFunction";
 static const char DeletedNotPublic[] = "DeletedNotPublic";
 
+UseEqualsDeleteCheck::UseEqualsDeleteCheck(StringRef Name,
+                                           ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)) {}
+
 void UseEqualsDeleteCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IgnoreMacros", IgnoreMacros);
 }
 
 void UseEqualsDeleteCheck::registerMatchers(MatchFinder *Finder) {
-  auto PrivateSpecialFn = cxxMethodDecl(
-      isPrivate(),
-      anyOf(cxxConstructorDecl(anyOf(isDefaultConstructor(),
-                                     isCopyConstructor(), isMoveConstructor())),
-            cxxMethodDecl(
-                anyOf(isCopyAssignmentOperator(), isMoveAssignmentOperator())),
-            cxxDestructorDecl()));
+  auto PrivateSpecialFn = cxxMethodDecl(isPrivate(), isSpecialFunction());
 
   Finder->addMatcher(
       cxxMethodDecl(
-          PrivateSpecialFn,
-          unless(anyOf(hasAnyBody(stmt()), isDefaulted(), isDeleted(),
-                       ast_matchers::isTemplateInstantiation(),
-                       // Ensure that all methods except private special member
-                       // functions are defined.
-                       hasParent(cxxRecordDecl(hasMethod(unless(
-                           anyOf(PrivateSpecialFn, hasAnyBody(stmt()), isPure(),
-                                 isDefaulted(), isDeleted()))))))))
+          PrivateSpecialFn, unless(hasAnyDefinition()), unless(isUsed()),
+          // Ensure that all methods except private special member functions are
+          // defined.
+          unless(ofClass(hasMethod(cxxMethodDecl(unless(PrivateSpecialFn),
+                                                 unless(hasAnyDefinition()))))))
           .bind(SpecialFunction),
       this);
 
@@ -55,7 +76,7 @@ void UseEqualsDeleteCheck::check(const MatchFinder::MatchResult &Result) {
     SourceLocation EndLoc = Lexer::getLocForEndOfToken(
         Func->getEndLoc(), 0, *Result.SourceManager, getLangOpts());
 
-    if (Func->getLocation().isMacroID() && IgnoreMacros)
+    if (IgnoreMacros && Func->getLocation().isMacroID())
       return;
     // FIXME: Improve FixItHint to make the method public.
     diag(Func->getLocation(),
@@ -66,7 +87,7 @@ void UseEqualsDeleteCheck::check(const MatchFinder::MatchResult &Result) {
     // Ignore this warning in macros, since it's extremely noisy in code using
     // DISALLOW_COPY_AND_ASSIGN-style macros and there's no easy way to
     // automatically fix the warning when macros are in play.
-    if (Func->getLocation().isMacroID() && IgnoreMacros)
+    if (IgnoreMacros && Func->getLocation().isMacroID())
       return;
     // FIXME: Add FixItHint to make the method public.
     diag(Func->getLocation(), "deleted member function should be public");

diff --git a/clang-tools-extra/clang-tidy/modernize/UseEqualsDeleteCheck.h b/clang-tools-extra/clang-tidy/modernize/UseEqualsDeleteCheck.h
@@ -34,15 +34,16 @@ namespace clang::tidy::modernize {
 /// http://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-delete.html
 class UseEqualsDeleteCheck : public ClangTidyCheck {
 public:
-  UseEqualsDeleteCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context),
-        IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)) {}
+  UseEqualsDeleteCheck(StringRef Name, ClangTidyContext *Context);
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
 
 private:
   const bool IgnoreMacros;

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -234,6 +234,10 @@ Changes in existing checks
   <clang-tidy/checks/modernize/loop-convert>` to support for-loops with
   iterators initialized by free functions like ``begin``, ``end``, or ``size``.
 
+- Improved :doc:`modernize-use-equals-delete
+  <clang-tidy/checks/modernize/use-equals-delete>` check to ignore
+  false-positives when special member function is actually used or implicit.
+
 - Improved :doc:`modernize-use-std-print
   <clang-tidy/checks/modernize/use-std-print>` check to accurately generate
   fixes for reordering arguments.

diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-equals-delete.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-equals-delete.cpp
@@ -191,3 +191,31 @@ class C {
 private:
   MACRO(C);
 };
+
+namespace PR33759 {
+
+  class Number {
+    private:
+      Number();
+      ~Number();
+
+    public:
+      static Number& getNumber() {
+        static Number number;
+        return number;
+      }
+
+      int getIntValue() { return (int)someFloat; }
+      float getFloatValue() { return someFloat; }
+    private:
+      float someFloat;
+  };
+
+  class Number2 {
+    private:
+      Number2();
+      ~Number2();
+    public:
+      static Number& getNumber();
+  };
+}