[clang-tools-extra] eb87e55 - [clang-tidy] Correct union & macros handling in modernize-use-equals-default

[Piotr Zegar via cfe-commits]

Author: Piotr Zegar
Date: 2023-03-26T13:22:44Z
New Revision: eb87e55c9aade5d7b42606487e69b5f3a6da1e65

URL: https://github.com/llvm/llvm-project/commit/eb87e55c9aade5d7b42606487e69b5f3a6da1e65
DIFF: https://github.com/llvm/llvm-project/commit/eb87e55c9aade5d7b42606487e69b5f3a6da1e65.diff

LOG: [clang-tidy] Correct union & macros handling in modernize-use-equals-default

To this moment this check were ignoring only inline
union special members, From now also out-of-line
special members going to be ignored. Also extended
support for IgnoreMacros to cover also macros used
inside a body, or used preprocesor directives.

Fixes:
 - https://github.com/llvm/llvm-project/issues/28300
 - https://github.com/llvm/llvm-project/issues/40554

Reviewed By: alexander-shaposhnikov

Differential Revision: https://reviews.llvm.org/D146882

Added: 
    

Modified: 
    clang-tools-extra/clang-tidy/modernize/UseEqualsDefaultCheck.cpp
    clang-tools-extra/docs/ReleaseNotes.rst
    clang-tools-extra/docs/clang-tidy/checks/modernize/use-equals-default.rst
    clang-tools-extra/test/clang-tidy/checkers/modernize/use-equals-default.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/modernize/UseEqualsDefaultCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseEqualsDefaultCheck.cpp
index d48d43f2677f0..afac9f7497ac8 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseEqualsDefaultCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseEqualsDefaultCheck.cpp
@@ -235,19 +235,19 @@ void UseEqualsDefaultCheck::registerMatchers(MatchFinder *Finder) {
 
   // Destructor.
   Finder->addMatcher(
-      cxxDestructorDecl(unless(hasParent(IsUnionLikeClass)), isDefinition())
+      cxxDestructorDecl(isDefinition(), unless(ofClass(IsUnionLikeClass)))
           .bind(SpecialFunction),
       this);
+  // Constructor.
   Finder->addMatcher(
       cxxConstructorDecl(
-          unless(
-              hasParent(decl(anyOf(IsUnionLikeClass, functionTemplateDecl())))),
-          isDefinition(),
+          isDefinition(), unless(ofClass(IsUnionLikeClass)),
+          unless(hasParent(functionTemplateDecl())),
           anyOf(
               // Default constructor.
-              allOf(unless(hasAnyConstructorInitializer(isWritten())),
-                    unless(isVariadic()), parameterCountIs(0),
-                    IsPublicOrOutOfLineUntilCPP20),
+              allOf(parameterCountIs(0),
+                    unless(hasAnyConstructorInitializer(isWritten())),
+                    unless(isVariadic()), IsPublicOrOutOfLineUntilCPP20),
               // Copy constructor.
               allOf(isCopyConstructor(),
                     // Discard constructors that can be used as a copy
@@ -258,9 +258,9 @@ void UseEqualsDefaultCheck::registerMatchers(MatchFinder *Finder) {
       this);
   // Copy-assignment operator.
   Finder->addMatcher(
-      cxxMethodDecl(unless(hasParent(
-                        decl(anyOf(IsUnionLikeClass, functionTemplateDecl())))),
-                    isDefinition(), isCopyAssignmentOperator(),
+      cxxMethodDecl(isDefinition(), isCopyAssignmentOperator(),
+                    unless(ofClass(IsUnionLikeClass)),
+                    unless(hasParent(functionTemplateDecl())),
                     // isCopyAssignmentOperator() allows the parameter to be
                     // passed by value, and in this case it cannot be
                     // defaulted.
@@ -299,6 +299,12 @@ void UseEqualsDefaultCheck::check(const MatchFinder::MatchResult &Result) {
   if (!SpecialFunctionDecl->isCopyAssignmentOperator() && !Body->body_empty())
     return;
 
+  // If body contain any preprocesor derictives, don't warn.
+  if (IgnoreMacros && utils::lexer::rangeContainsExpansionsOrDirectives(
+                          Body->getSourceRange(), *Result.SourceManager,
+                          Result.Context->getLangOpts()))
+    return;
+
   // If there are comments inside the body, don't do the change.
   bool ApplyFix = SpecialFunctionDecl->isCopyAssignmentOperator() ||
                   bodyEmpty(Result.Context, Body);

diff  --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 38a64616410ac..1a53aac8fecf4 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -203,6 +203,11 @@ Changes in existing checks
   constructors toward hand written constructors so that they are skipped if more
   than one exists.
 
+- Fixed false positive in :doc:`modernize-use-equals-default
+  <clang-tidy/checks/modernize/use-equals-default>` check for special member
+  functions containing macros or preprocessor directives, and out-of-line special
+  member functions in unions.
+
 - Fixed reading `HungarianNotation.CString.*` options in
   :doc:`readability-identifier-naming
   <clang-tidy/checks/readability/identifier-naming>` check.

diff  --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-equals-default.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-equals-default.rst
index 5509a5414f8c5..7bd6fa93ba1ea 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-equals-default.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-equals-default.rst
@@ -32,5 +32,6 @@ Options
 
 .. option:: IgnoreMacros
 
-   If set to `true`, the check will not give warnings inside macros. Default
-   is `true`.
+   If set to `true`, the check will not give warnings inside macros and will
+   ignore special members with bodies contain macros or preprocessor directives.
+   Default is `true`.

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-equals-default.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-equals-default.cpp
index cc5d379b3c3de..a1d6c25e6364a 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-equals-default.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-equals-default.cpp
@@ -42,6 +42,18 @@ union NU {
   NE Field;
 };
 
+// Skip unions with out-of-line constructor/destructor.
+union NUO {
+  NUO();
+  ~NUO();
+  NE Field;
+};
+
+NUO::NUO() {}
+// CHECK-FIXES: NUO::NUO() {}
+NUO::~NUO() {}
+// CHECK-FIXES: NUO::~NUO() {}
+
 // Skip structs/classes containing anonymous unions.
 struct SU {
   SU() {}
@@ -266,3 +278,21 @@ OTC::~OTC() try {} catch(...) {}
   };
 
 STRUCT_WITH_DEFAULT(unsigned char, InMacro)
+
+#define ZERO_VALUE 0
+struct PreprocesorDependentTest
+{
+  void something();
+
+  PreprocesorDependentTest() {
+#if ZERO_VALUE
+    something();
+#endif
+  }
+
+  ~PreprocesorDependentTest() {
+#if ZERO_VALUE
+    something();
+#endif
+  }
+};