https://github.com/your-moon updated https://github.com/llvm/llvm-project/pull/232163
>From 0193069b22b300690b29203c453722d099fe1336 Mon Sep 17 00:00:00 2001 From: Munkherdene <[email protected]> Date: Wed, 14 Oct 2026 23:18:10 +0800 Subject: [PATCH 1/4] [clang-tidy] Fix crash in bugprone-crtp-constructor-accessibility with macros This check crashes on assertion builds when a constructor starts with a macro, like Boost's `BOOST_CXX14_CONSTEXPR`. Smallest repro I found: ```cpp #define CONSTEXPR constexpr template <typename T> class CRTP { public: CONSTEXPR CRTP() {} }; class A : CRTP<A> {}; ``` The fix-it puts `private:` at `getBeginLoc().getLocWithOffset(-1)`. Here the begin loc is the first token of the macro expansion, so -1 steps out of that SLocEntry into the one before it. If that one is a file, `isMacroArgExpansion` hits the "Not a macro expansion SLocEntry!" assert. Now the begin loc is mapped out of the macro with `Lexer::isAtStartOfMacroExpansion`, so `private:` goes before `CONSTEXPR`. If the constructor starts in the middle of a macro there's no good place for it, so I skip the fix-it there, and the `friend` one too. Otherwise `--fix` would add `friend T;` and leave the constructor public. The test is in its own file on purpose. The crash only shows up when a file entry sits right before the macro expansion, and in the existing test file an earlier `CRTP<A<T>>` splits `>>` into an expansion entry, which hides it. Fixes #145093 --- .../CrtpConstructorAccessibilityCheck.cpp | 31 +++++++++++------ clang-tools-extra/docs/ReleaseNotes.md | 4 +++ .../crtp-constructor-accessibility-macro.cpp | 34 +++++++++++++++++++ 3 files changed, 58 insertions(+), 11 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp index 13d9750234b0d4..87e22892223683 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp @@ -67,18 +67,27 @@ getDerivedParameter(const ClassTemplateSpecializationDecl *CRTP, static std::vector<FixItHint> hintMakeCtorPrivate(const CXXConstructorDecl *Ctor, const std::string &OriginalAccess) { - std::vector<FixItHint> Hints; - - Hints.emplace_back(FixItHint::CreateInsertion( - Ctor->getBeginLoc().getLocWithOffset(-1), "private:\n")); - const ASTContext &ASTCtx = Ctor->getASTContext(); + const SourceManager &SM = ASTCtx.getSourceManager(); + const LangOptions &LangOpts = ASTCtx.getLangOpts(); + + // Stepping back from a macro location can leave its expansion, so insert + // before the macro when the constructor starts at one, as in + // 'CONSTEXPR CRTP() {}', and give no fix-it otherwise. + SourceLocation CtorBeginLoc = Ctor->getBeginLoc(); + if (CtorBeginLoc.isMacroID() && + !Lexer::isAtStartOfMacroExpansion(CtorBeginLoc, SM, LangOpts, + &CtorBeginLoc)) + return {}; + const SourceLocation CtorEndLoc = Ctor->isExplicitlyDefaulted() - ? utils::lexer::findNextTerminator(Ctor->getEndLoc(), - ASTCtx.getSourceManager(), - ASTCtx.getLangOpts()) + ? utils::lexer::findNextTerminator(Ctor->getEndLoc(), SM, LangOpts) : Ctor->getEndLoc(); + + std::vector<FixItHint> Hints; + Hints.emplace_back(FixItHint::CreateInsertion( + CtorBeginLoc.getLocWithOffset(-1), "private:\n")); Hints.emplace_back(FixItHint::CreateInsertion( CtorEndLoc.getLocWithOffset(1), '\n' + OriginalAccess + ':' + '\n')); @@ -159,15 +168,15 @@ void CrtpConstructorAccessibilityCheck::check( const bool IsPublic = Ctor->getAccess() == AS_public; const std::string Access = IsPublic ? "public" : "protected"; + const std::vector<FixItHint> CtorHints = hintMakeCtorPrivate(Ctor, Access); WithFriendHintIfNeeded( diag(Ctor->getLocation(), "%0 constructor allows the CRTP to be %select{inherited " "from|constructed}1 as a regular template class; consider making " "it private%select{| and declaring the derived class as friend}2") - << Access << IsPublic << NeedsFriend - << hintMakeCtorPrivate(Ctor, Access), - NeedsFriend); + << Access << IsPublic << NeedsFriend << CtorHints, + NeedsFriend && !CtorHints.empty()); } } diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index 14480052535567..c0ec1866bd07eb 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -174,6 +174,10 @@ infrastructure are described first, followed by tool-specific sections. #### Changes in existing checks +- Fixed a crash in {doc}`bugprone-crtp-constructor-accessibility + <clang-tidy/checks/bugprone/crtp-constructor-accessibility>` when a + constructor declaration begins with a macro. + - Improved {doc}`bugprone-easily-swappable-parameters <clang-tidy/checks/bugprone/easily-swappable-parameters>` check by no longer emitting empty notes for type aliases when {option}`ModelImplicitConversions` diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp new file mode 100644 index 00000000000000..0dc982caf6ba84 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp @@ -0,0 +1,34 @@ +// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-crtp-constructor-accessibility %t -- -- -fno-delayed-template-parsing + +// Keep 'CONSTEXPR' the first expansion in the file. The crash needs a file +// entry right before it, and a split '>>' or another macro above would hide it. +#define CONSTEXPR constexpr + +namespace ctor_begins_with_macro { +template <typename T> +class CRTP { +public: + CONSTEXPR CRTP() {} + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: public constructor allows the CRTP to be constructed as a regular template class; consider making it private and declaring the derived class as friend [bugprone-crtp-constructor-accessibility] + // CHECK-FIXES: private:{{[[:space:]]*}}CONSTEXPR CRTP() {}{{[[:space:]]*}}public: + // CHECK-FIXES: friend T; +}; + +class A : CRTP<A> {}; +} // namespace ctor_begins_with_macro + +#define MEMBERS(Name) int I; Name() {} + +namespace ctor_inside_macro { +template <typename T> +class CRTP { +public: + MEMBERS(CRTP) + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: public constructor allows the CRTP to be constructed as a regular template class; consider making it private and declaring the derived class as friend [bugprone-crtp-constructor-accessibility] + // CHECK-FIXES: MEMBERS(CRTP) + // CHECK-FIXES-NOT: friend T; +}; + +// CHECK-FIXES: class A : CRTP<A> {}; +class A : CRTP<A> {}; +} // namespace ctor_inside_macro >From fba90f16c153c460b20a4cec2bceabeac81a27b1 Mon Sep 17 00:00:00 2001 From: Munkherdene <[email protected]> Date: Thu, 15 Oct 2026 10:33:08 +0800 Subject: [PATCH 2/4] Tighten comments and drop a redundant test anchor --- .../bugprone/CrtpConstructorAccessibilityCheck.cpp | 3 +-- .../bugprone/crtp-constructor-accessibility-macro.cpp | 5 ++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp index 87e22892223683..972c7d002fb9bf 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp @@ -72,8 +72,7 @@ hintMakeCtorPrivate(const CXXConstructorDecl *Ctor, const LangOptions &LangOpts = ASTCtx.getLangOpts(); // Stepping back from a macro location can leave its expansion, so insert - // before the macro when the constructor starts at one, as in - // 'CONSTEXPR CRTP() {}', and give no fix-it otherwise. + // before the macro, or give up if the constructor starts inside one. SourceLocation CtorBeginLoc = Ctor->getBeginLoc(); if (CtorBeginLoc.isMacroID() && !Lexer::isAtStartOfMacroExpansion(CtorBeginLoc, SM, LangOpts, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp index 0dc982caf6ba84..baacb6c572c67f 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp @@ -1,7 +1,7 @@ // RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-crtp-constructor-accessibility %t -- -- -fno-delayed-template-parsing -// Keep 'CONSTEXPR' the first expansion in the file. The crash needs a file -// entry right before it, and a split '>>' or another macro above would hide it. +// CONSTEXPR must stay the first expansion in this file: the crash needs a file +// entry right before it, and a macro or a split '>>' above would hide it. #define CONSTEXPR constexpr namespace ctor_begins_with_macro { @@ -29,6 +29,5 @@ class CRTP { // CHECK-FIXES-NOT: friend T; }; -// CHECK-FIXES: class A : CRTP<A> {}; class A : CRTP<A> {}; } // namespace ctor_inside_macro >From 2a67c3b6c48f1c7f01c669bc4b23beb42b97c3cc Mon Sep 17 00:00:00 2001 From: Munkherdene <[email protected]> Date: Thu, 15 Oct 2026 11:31:53 +0800 Subject: [PATCH 3/4] Insert at token boundaries and map both ends out of macros --- .../CrtpConstructorAccessibilityCheck.cpp | 21 ++++++++-------- clang-tools-extra/docs/ReleaseNotes.md | 3 ++- .../crtp-constructor-accessibility-macro.cpp | 25 +++++++++++-------- .../crtp-constructor-accessibility.cpp | 12 +++++++++ 4 files changed, 39 insertions(+), 22 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp index 972c7d002fb9bf..87a3e830701bec 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp @@ -71,26 +71,25 @@ hintMakeCtorPrivate(const CXXConstructorDecl *Ctor, const SourceManager &SM = ASTCtx.getSourceManager(); const LangOptions &LangOpts = ASTCtx.getLangOpts(); - // Stepping back from a macro location can leave its expansion, so insert - // before the macro, or give up if the constructor starts inside one. + // Insert right before the first token and after the last one, mapped out of + // macros. Give up if either end is in the middle of a macro expansion. SourceLocation CtorBeginLoc = Ctor->getBeginLoc(); if (CtorBeginLoc.isMacroID() && !Lexer::isAtStartOfMacroExpansion(CtorBeginLoc, SM, LangOpts, &CtorBeginLoc)) return {}; - const SourceLocation CtorEndLoc = + const SourceLocation CtorEndLoc = Lexer::getLocForEndOfToken( Ctor->isExplicitlyDefaulted() ? utils::lexer::findNextTerminator(Ctor->getEndLoc(), SM, LangOpts) - : Ctor->getEndLoc(); - - std::vector<FixItHint> Hints; - Hints.emplace_back(FixItHint::CreateInsertion( - CtorBeginLoc.getLocWithOffset(-1), "private:\n")); - Hints.emplace_back(FixItHint::CreateInsertion( - CtorEndLoc.getLocWithOffset(1), '\n' + OriginalAccess + ':' + '\n')); + : Ctor->getEndLoc(), + 0, SM, LangOpts); + if (CtorEndLoc.isInvalid()) + return {}; - return Hints; + return {FixItHint::CreateInsertion(CtorBeginLoc, "private:\n"), + FixItHint::CreateInsertion(CtorEndLoc, + '\n' + OriginalAccess + ':' + '\n')}; } void CrtpConstructorAccessibilityCheck::registerMatchers(MatchFinder *Finder) { diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index c0ec1866bd07eb..85ae46faee6dd3 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -176,7 +176,8 @@ infrastructure are described first, followed by tool-specific sections. - Fixed a crash in {doc}`bugprone-crtp-constructor-accessibility <clang-tidy/checks/bugprone/crtp-constructor-accessibility>` when a - constructor declaration begins with a macro. + constructor begins with a macro, and fix-its that inserted `private:` into + the preceding token, comment or macro argument. - Improved {doc}`bugprone-easily-swappable-parameters <clang-tidy/checks/bugprone/easily-swappable-parameters>` check by no longer diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp index baacb6c572c67f..2bd0966c66ec1b 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp @@ -3,31 +3,36 @@ // CONSTEXPR must stay the first expansion in this file: the crash needs a file // entry right before it, and a macro or a split '>>' above would hide it. #define CONSTEXPR constexpr +#define EMPTY_BODY {} -namespace ctor_begins_with_macro { +namespace ctor_begins_and_ends_with_macro { template <typename T> class CRTP { public: - CONSTEXPR CRTP() {} + CONSTEXPR CRTP() EMPTY_BODY // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: public constructor allows the CRTP to be constructed as a regular template class; consider making it private and declaring the derived class as friend [bugprone-crtp-constructor-accessibility] - // CHECK-FIXES: private:{{[[:space:]]*}}CONSTEXPR CRTP() {}{{[[:space:]]*}}public: + // CHECK-FIXES: private:{{[[:space:]]*}}CONSTEXPR CRTP() EMPTY_BODY{{[[:space:]]*}}public: // CHECK-FIXES: friend T; }; class A : CRTP<A> {}; -} // namespace ctor_begins_with_macro +} // namespace ctor_begins_and_ends_with_macro -#define MEMBERS(Name) int I; Name() {} +#define MEMBER_THEN(X) int I; X +#define THEN_MEMBER(X) X int J; -namespace ctor_inside_macro { +namespace ctor_in_middle_of_macro { template <typename T> class CRTP { public: - MEMBERS(CRTP) - // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: public constructor allows the CRTP to be constructed as a regular template class; consider making it private and declaring the derived class as friend [bugprone-crtp-constructor-accessibility] - // CHECK-FIXES: MEMBERS(CRTP) + MEMBER_THEN(CRTP() {}) + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: public constructor allows the CRTP to be constructed as a regular template class; consider making it private and declaring the derived class as friend [bugprone-crtp-constructor-accessibility] + // CHECK-FIXES: MEMBER_THEN(CRTP() {}) + THEN_MEMBER(CRTP(int) {}) + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: public constructor allows the CRTP to be constructed as a regular template class; consider making it private and declaring the derived class as friend [bugprone-crtp-constructor-accessibility] + // CHECK-FIXES: THEN_MEMBER(CRTP(int) {}) // CHECK-FIXES-NOT: friend T; }; class A : CRTP<A> {}; -} // namespace ctor_inside_macro +} // namespace ctor_in_middle_of_macro diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp index 44cfcd136f2387..8326bc0f35b572 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp @@ -284,3 +284,15 @@ class CRTP class A : CRTP<A> {}; } // namespace public_copy_protected_move_constructor_deleted + +namespace ctor_right_after_access_specifier { +template <typename T> +class CRTP { +public:CRTP() {} + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: public constructor allows the CRTP to be constructed as a regular template class; consider making it private and declaring the derived class as friend [bugprone-crtp-constructor-accessibility] + // CHECK-FIXES: public:private:{{[[:space:]]*}}CRTP() {}{{[[:space:]]*}}public: + // CHECK-FIXES: friend T; +}; + +class A : CRTP<A> {}; +} // namespace ctor_right_after_access_specifier >From 5fef8b6a1bf8212d8258db49cac05068641e15be Mon Sep 17 00:00:00 2001 From: Munkherdene <[email protected]> Date: Sun, 25 Oct 2026 19:11:08 +0800 Subject: [PATCH 4/4] Split CHECK-FIXES into CHECK-FIXES-NEXT lines --- .../bugprone/crtp-constructor-accessibility-macro.cpp | 4 +++- .../checkers/bugprone/crtp-constructor-accessibility.cpp | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp index 2bd0966c66ec1b..94539bc9d06cdc 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp @@ -11,7 +11,9 @@ class CRTP { public: CONSTEXPR CRTP() EMPTY_BODY // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: public constructor allows the CRTP to be constructed as a regular template class; consider making it private and declaring the derived class as friend [bugprone-crtp-constructor-accessibility] - // CHECK-FIXES: private:{{[[:space:]]*}}CONSTEXPR CRTP() EMPTY_BODY{{[[:space:]]*}}public: + // CHECK-FIXES: private: + // CHECK-FIXES-NEXT: CONSTEXPR CRTP() EMPTY_BODY + // CHECK-FIXES-NEXT: public: // CHECK-FIXES: friend T; }; diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp index 8326bc0f35b572..a9a677b443afae 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp @@ -290,7 +290,9 @@ template <typename T> class CRTP { public:CRTP() {} // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: public constructor allows the CRTP to be constructed as a regular template class; consider making it private and declaring the derived class as friend [bugprone-crtp-constructor-accessibility] - // CHECK-FIXES: public:private:{{[[:space:]]*}}CRTP() {}{{[[:space:]]*}}public: + // CHECK-FIXES: public:private: + // CHECK-FIXES-NEXT: CRTP() {} + // CHECK-FIXES-NEXT: public: // CHECK-FIXES: friend T; }; _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
