[clang-tidy] add 'IgnoreMarcos' option to 'special-member-functions' check #143550

vbvictor · 2025-06-10T15:23:04Z

No description provided.

llvmbot · 2025-06-10T15:23:36Z

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Baranov Victor (vbvictor)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/143550.diff

6 Files Affected:

(modified) clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp (+12-3)
(modified) clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h (+1)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+8)
(modified) clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/special-member-functions.rst (+5)
(added) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions-macros.cpp (+57)
(modified) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions.cpp (+26)

diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
index 0de143dbb1b89..0b6b8d9c97135 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
@@ -18,6 +18,12 @@ using namespace clang::ast_matchers;
 
 namespace clang::tidy::cppcoreguidelines {
 
+namespace {
+AST_MATCHER(CXXRecordDecl, isInMacro) {
+  return Node.getBeginLoc().isMacroID() && Node.getEndLoc().isMacroID();
+}
+} // namespace
+
 SpecialMemberFunctionsCheck::SpecialMemberFunctionsCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context), AllowMissingMoveFunctions(Options.get(
@@ -26,7 +32,8 @@ SpecialMemberFunctionsCheck::SpecialMemberFunctionsCheck(
       AllowMissingMoveFunctionsWhenCopyIsDeleted(
           Options.get("AllowMissingMoveFunctionsWhenCopyIsDeleted", false)),
       AllowImplicitlyDeletedCopyOrMove(
-          Options.get("AllowImplicitlyDeletedCopyOrMove", false)) {}
+          Options.get("AllowImplicitlyDeletedCopyOrMove", false)),
+      IgnoreMacros(Options.get("IgnoreMacros", true)) {}
 
 void SpecialMemberFunctionsCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
@@ -36,6 +43,7 @@ void SpecialMemberFunctionsCheck::storeOptions(
                 AllowMissingMoveFunctionsWhenCopyIsDeleted);
   Options.store(Opts, "AllowImplicitlyDeletedCopyOrMove",
                 AllowImplicitlyDeletedCopyOrMove);
+  Options.store(Opts, "IgnoreMacros", IgnoreMacros);
 }
 
 std::optional<TraversalKind>
@@ -45,11 +53,12 @@ SpecialMemberFunctionsCheck::getCheckTraversalKind() const {
 }
 
 void SpecialMemberFunctionsCheck::registerMatchers(MatchFinder *Finder) {
-  auto IsNotImplicitOrDeleted = anyOf(unless(isImplicit()), isDeleted());
+  const auto IsNotImplicitOrDeleted = anyOf(unless(isImplicit()), isDeleted());
+  const ast_matchers::internal::Matcher<CXXRecordDecl> Anything = anything();
 
   Finder->addMatcher(
       cxxRecordDecl(
-          unless(isImplicit()),
+          unless(isImplicit()), IgnoreMacros ? unless(isInMacro()) : Anything,
           eachOf(has(cxxDestructorDecl(unless(isImplicit())).bind("dtor")),
                  has(cxxConstructorDecl(isCopyConstructor(),
                                         IsNotImplicitOrDeleted)
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
index dee01cb5a9fdd..c18ed7db055ba 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
@@ -69,6 +69,7 @@ class SpecialMemberFunctionsCheck : public ClangTidyCheck {
   const bool AllowMissingMoveFunctionsWhenCopyIsDeleted;
   const bool AllowImplicitlyDeletedCopyOrMove;
   ClassDefiningSpecialMembersMap ClassWithSpecialMembers;
+  const bool IgnoreMacros;
 };
 
 } // namespace clang::tidy::cppcoreguidelines
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 19ccd1790e757..a79c10ed15a3d 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -191,11 +191,19 @@ Changes in existing checks
   <clang-tidy/checks/concurrency/mt-unsafe>` check by fixing a false positive
   where ``strerror`` was flagged as MT-unsafe.
 
+- Improved :doc:`cppcoreguidelines-special-member-functions
+  <clang-tidy/checks/cppcoreguidelines/special-member-functions>` check by
+  adding the option `IgnoreMacros` to ignore classes defined in macros.
+
 - Improved :doc:`google-readability-namespace-comments
   <clang-tidy/checks/google/readability-namespace-comments>` check by adding
   the option `AllowOmittingNamespaceComments` to accept if a namespace comment
   is omitted entirely.
 
+- Improved :doc:`hicpp-special-member-functions
+  <clang-tidy/checks/hicpp/special-member-functions>` check by adding the
+  option `IgnoreMacros` to ignore classes defined in macros.
+
 - Improved :doc:`llvm-namespace-comment
   <clang-tidy/checks/llvm/namespace-comment>` check by adding the option
   `AllowOmittingNamespaceComments` to accept if a namespace comment is omitted
diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/special-member-functions.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/special-member-functions.rst
index 20f898fdab930..982d16fc8d23d 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/special-member-functions.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/special-member-functions.rst
@@ -85,3 +85,8 @@ Options
       struct A : boost::noncopyable {
         ~A() { std::cout << "dtor\n"; }
       };
+
+.. option:: IgnoreMacros
+
+   If set to `true`, the check will not give warnings for classes defined
+   inside macros. Default is `true`.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions-macros.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions-macros.cpp
new file mode 100644
index 0000000000000..58198979203ec
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions-macros.cpp
@@ -0,0 +1,57 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t -- -config="{CheckOptions: {cppcoreguidelines-special-member-functions.IgnoreMacros: false}}" --
+
+class DefinesDestructor {
+  ~DefinesDestructor();
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesDefaultedDestructor {
+  ~DefinesDefaultedDestructor() = default;
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDefaultedDestructor' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesCopyConstructor {
+  DefinesCopyConstructor(const DefinesCopyConstructor &);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyConstructor' defines a copy constructor but does not define a destructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesNothing {
+};
+
+class DefinesEverything {
+  DefinesEverything(const DefinesEverything &);
+  DefinesEverything &operator=(const DefinesEverything &);
+  DefinesEverything(DefinesEverything &&);
+  DefinesEverything &operator=(DefinesEverything &&);
+  ~DefinesEverything();
+};
+
+#define DEFINE_DESTRUCTOR_ONLY(ClassName) \
+  class ClassName { \
+    ~ClassName(); \
+  };
+
+#define DEFINE_COPY_CTOR_ONLY(ClassName) \
+  class ClassName { \
+    ClassName(const ClassName &); \
+  };
+
+#define DEFINE_CLASS_WITH_DTOR(ClassName) \
+  class ClassName { \
+    ~ClassName(); \
+  };
+
+DEFINE_DESTRUCTOR_ONLY(MacroDefinedClass1)
+// CHECK-MESSAGES: [[@LINE-1]]:24: warning: class 'MacroDefinedClass1' defines a destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator
+DEFINE_COPY_CTOR_ONLY(MacroDefinedClass2)
+// CHECK-MESSAGES: [[@LINE-1]]:23: warning: class 'MacroDefinedClass2' defines a copy constructor but does not define a destructor, a copy assignment operator, a move constructor or a move assignment operator
+DEFINE_CLASS_WITH_DTOR(MacroDefinedClass3)
+// CHECK-MESSAGES: [[@LINE-1]]:24: warning: class 'MacroDefinedClass3' defines a destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator
+
+// Test partial macro expansion
+#define CLASS_NAME MacroNamedClass
+class CLASS_NAME {
+  ~MacroNamedClass();
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'MacroNamedClass' defines a destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator
+
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions.cpp
index 60c945c8e20c3..28b515fb6afa3 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions.cpp
@@ -70,3 +70,29 @@ struct TemplateClass {
 // This should not cause problems.
 TemplateClass<int> InstantiationWithInt;
 TemplateClass<double> InstantiationWithDouble;
+
+#define DEFINE_DESTRUCTOR_ONLY(ClassName) \
+  class ClassName { \
+    ~ClassName(); \
+  };
+
+#define DEFINE_COPY_CTOR_ONLY(ClassName) \
+  class ClassName { \
+    ClassName(const ClassName &); \
+  };
+
+#define DEFINE_CLASS_WITH_DTOR(ClassName) \
+  class ClassName { \
+    ~ClassName(); \
+  };
+
+DEFINE_DESTRUCTOR_ONLY(MacroDefinedClass1)
+DEFINE_COPY_CTOR_ONLY(MacroDefinedClass2)
+DEFINE_CLASS_WITH_DTOR(MacroDefinedClass3)
+
+// Test partial macro expansion
+#define CLASS_NAME MacroNamedClass
+class CLASS_NAME {
+  ~MacroNamedClass();
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'MacroNamedClass' defines a destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator

5chmidti

LGTM but I'm +- on the default

Let's see if others have a stronger opinion on the default

Not directly part of this, but:
I'd prefer to see a warning for expansions of macros defined inside the code base, but not from macros defined outside of it. (E.g. MY_CLASS vs gtest macros)

5chmidti · 2025-06-21T11:30:30Z

Actually, the cxxRecordDecl might not be in a macro, but the special member functions are.
E.g.:
``` cpp
#define DEF_COPY(name)
name(const name&) = default;
name& operator=(const name&) = default;
` ` `

vbvictor · 2025-06-21T17:50:52Z

Actually, the cxxRecordDecl might not be in a macro, but the special member functions are.

The checker doc states:
The check finds classes where some but not all of the special member functions are defined.
I think we only need to ignore if the whole class expands from macro. If not, I don't see a reason why a user can't provided needed members.

I'd prefer to see a warning for expansions of macros defined inside the code base

I think we could just limit the scope of the whole check by unless(isExpansionInSystemHeader()).
There was some work by carlosgalvezp to ignore system headers in all checks by default so maybe this addition will become obsolete soon.

I'm +- on the default

FYI there is a total of 22 checks that use IgnoreMacros option, 17 of them have true by default and 5 have false

carlosgalvezp · 2025-06-23T06:51:41Z

clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp

@@ -18,6 +18,12 @@ using namespace clang::ast_matchers;

 namespace clang::tidy::cppcoreguidelines {

+namespace {
+AST_MATCHER(CXXRecordDecl, isInMacro) {


(For another commit) This comes up often, maybe we can create an AST matcher for this that can be reused?

Yes, I was thinking lately about it.
I've seen some matchers use only Node.getBeginLoc().isMacroID() others use Node.getBeginLoc().isMacroID() && Node.getEndLoc().isMacroID() so we need to decide on the wanted behavior.
I guess we should place both Begin and End inside a macro

carlosgalvezp

LGTM, don't have strong opinions on the default.

…check

5chmidti · 2025-06-23T17:15:56Z

Actually, the cxxRecordDecl might not be in a macro, but the special member functions are.

The checker doc states:
The check finds classes where some but not all of the special member functions are defined.
I think we only need to ignore if the whole class expands from macro. If not, I don't see a reason why a user can't provided needed members.

Good point

I'd prefer to see a warning for expansions of macros defined inside the code base

I think we could just limit the scope of the whole check by unless(isExpansionInSystemHeader()).
There was some work by carlosgalvezp to ignore system headers in all checks by default so maybe this addition will become obsolete soon.

Yeah, this was just a thought. But that work would not be for this (I think). I meant macros defined in system headers, but with expansions in user code.

I'm +- on the default

FYI there is a total of 22 checks that use IgnoreMacros option, 17 of them have true by default and 5 have false

Then let's stay with what it is now (true). For things like goto, it makes IMO sense to use false because goto is a really bad pattern.

vbvictor · 2025-06-23T18:21:09Z

I meant macros defined in system headers, but with expansions in user code.

Make sense, I suppose we should make for that and potentially implement such logic in more checks.

I think we can land this as is for now

vbvictor requested review from PiotrZSL, carlosgalvezp, EugeneZelenko and HerrCai0907 June 10, 2025 15:23

llvmbot added clang-tools-extra clang-tidy labels Jun 10, 2025

5chmidti approved these changes Jun 21, 2025

View reviewed changes

carlosgalvezp reviewed Jun 23, 2025

View reviewed changes

carlosgalvezp approved these changes Jun 23, 2025

View reviewed changes

vbvictor added 3 commits June 23, 2025 16:37

[clang-tidy] add 'IgnoreMarcos' option to 'special-member-functions' …

897ef52

…check

fix newline

b05281f

fix docs

4f7d1b9

vbvictor force-pushed the ignore-macros-special-member-functions branch from 37c05f4 to 4f7d1b9 Compare June 23, 2025 13:38

vbvictor merged commit e435558 into llvm:main Jun 24, 2025
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang-tidy] add 'IgnoreMarcos' option to 'special-member-functions' check #143550

[clang-tidy] add 'IgnoreMarcos' option to 'special-member-functions' check #143550

Uh oh!

vbvictor commented Jun 10, 2025

Uh oh!

llvmbot commented Jun 10, 2025 •

edited

Loading

Uh oh!

5chmidti left a comment

Uh oh!

5chmidti commented Jun 21, 2025

Uh oh!

vbvictor commented Jun 21, 2025 •

edited

Loading

Uh oh!

carlosgalvezp Jun 23, 2025

Uh oh!

vbvictor Jun 23, 2025

Uh oh!

carlosgalvezp left a comment

Uh oh!

5chmidti commented Jun 23, 2025

Uh oh!

vbvictor commented Jun 23, 2025

Uh oh!

Uh oh!

Uh oh!

[clang-tidy] add 'IgnoreMarcos' option to 'special-member-functions' check #143550

[clang-tidy] add 'IgnoreMarcos' option to 'special-member-functions' check #143550

Uh oh!

Conversation

vbvictor commented Jun 10, 2025

Uh oh!

llvmbot commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

5chmidti left a comment

Choose a reason for hiding this comment

Uh oh!

5chmidti commented Jun 21, 2025

Uh oh!

vbvictor commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlosgalvezp Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

vbvictor Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

carlosgalvezp left a comment

Choose a reason for hiding this comment

Uh oh!

5chmidti commented Jun 23, 2025

Uh oh!

vbvictor commented Jun 23, 2025

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Jun 10, 2025 •

edited

Loading

vbvictor commented Jun 21, 2025 •

edited

Loading