Author: Gareth Andrew Lloyd Date: 2026-04-07T03:49:04Z New Revision: 49093c4a57ba64d384520ddd95a07c8b35fadcc0
URL: https://github.com/llvm/llvm-project/commit/49093c4a57ba64d384520ddd95a07c8b35fadcc0 DIFF: https://github.com/llvm/llvm-project/commit/49093c4a57ba64d384520ddd95a07c8b35fadcc0.diff LOG: [clang-tidy] Fix performance-trivially-destructible with C++20 modules (#178471) When a class definition is seen through both a header include and a C++20 module import, destructors may appear multiple times in the AST's redeclaration chain. The original matcher used `isFirstDecl()` which fails in this scenario because the same declaration can appear as both first and non-first depending on the view. Replace `unless(isFirstDecl())` with `isOutOfLine()` which correctly identifies out-of-line definitions by checking whether the lexical context differs from the semantic context. Also update clang-tools-extra's lit.cfg.py to call `use_clang()` instead of `clang_setup()` to make the `%clang` substitution available for tests. Fixes #178102 Co-authored-by: Chuanqi Xu <[email protected]> Added: clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible-module.cpp Modified: clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/test/CMakeLists.txt clang-tools-extra/test/lit.cfg.py Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp b/clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp index 416c41d7acd66..5088cfb7ca503 100644 --- a/clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp @@ -20,7 +20,11 @@ namespace clang::tidy::performance { namespace { -AST_MATCHER(Decl, isFirstDecl) { return Node.isFirstDecl(); } +// We use isOutOfLine() to find out-of-line defaulted destructor definitions. +// This is more robust than !isFirstDecl() because with C++20 modules, when a +// class is visible through both a header include and a module import, its +// declarations may appear multiple times in the redeclaration chain. +AST_MATCHER(CXXMethodDecl, isOutOfLine) { return Node.isOutOfLine(); } AST_MATCHER_P(CXXRecordDecl, hasBase, Matcher<QualType>, InnerMatcher) { return llvm::any_of(Node.bases(), [&](const CXXBaseSpecifier &BaseSpec) { @@ -33,8 +37,8 @@ AST_MATCHER_P(CXXRecordDecl, hasBase, Matcher<QualType>, InnerMatcher) { void TriviallyDestructibleCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( cxxDestructorDecl( - isDefaulted(), - unless(anyOf(isFirstDecl(), isVirtual(), + isDefaulted(), isOutOfLine(), + unless(anyOf(isVirtual(), ofClass(cxxRecordDecl( anyOf(hasBase(unless(isTriviallyDestructible())), has(fieldDecl(unless( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 02315415b975f..2558d5ea46d0d 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -404,6 +404,11 @@ Changes in existing checks - Fixes false negatives when using ``std::set`` from ``libstdc++``. +- Improved :doc:`performance-trivially-destructible + <clang-tidy/checks/performance/trivially-destructible>` check by fixing + false positives when a class is seen through both a header include and + a C++20 module import. + - Improved :doc:`readability-container-size-empty <clang-tidy/checks/readability/container-size-empty>` check: diff --git a/clang-tools-extra/test/CMakeLists.txt b/clang-tools-extra/test/CMakeLists.txt index 97c7a66cd69fd..22f227a891f82 100644 --- a/clang-tools-extra/test/CMakeLists.txt +++ b/clang-tools-extra/test/CMakeLists.txt @@ -44,6 +44,9 @@ set(CLANG_TOOLS_TEST_DEPS clang-resource-headers clang-tidy + + # Some tests invoke clang directly (e.g., to precompile modules). + clang ) # Add lit test dependencies. diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible-module.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible-module.cpp new file mode 100644 index 0000000000000..6e1b9ccb1e392 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible-module.cpp @@ -0,0 +1,61 @@ +// RUN: rm -rf %t +// RUN: mkdir -p %t +// RUN: split-file %s %t +// RUN: %clang -std=c++20 -x c++-module --precompile %t/mymodule.cppm -o %t/mymodule.pcm -I%t +// RUN: %check_clang_tidy -std=c++20 %t/main.cpp performance-trivially-destructible %t/out \ +// RUN: -- -- -std=c++20 -I%t -fmodule-file=mymodule=%t/mymodule.pcm + +// Test: C++20 modules - class visible through both #include and import. +// +// When a class is defined in a header that is both #include'd directly and +// included in a module's global module fragment, the class's destructor may +// appear multiple times in the redeclaration chain. +// +// This test verifies: +// 1. No false positive on in-line defaulted destructor (struct A) +// 2. No false positive on implicit destructor (struct X) +// 3. Correct warning on out-of-line defaulted destructor (struct B) +// +// We expect exactly 1 warning (for struct B's out-of-line destructor). + +//--- header.h +#pragma once + +// Negative cases: with modules, the destructor's redeclaration chain may +// contain multiple entries, which can trigger false positives if not handled +// correctly. + +// In-line defaulted destructor should NOT warn +struct A { + ~A() = default; +}; + +// Implicit destructor should NOT warn +struct X { + A a; +}; + +// Positive case: out-of-line defaulted destructor SHOULD warn +struct B { + ~B(); + int x; +}; + +//--- mymodule.cppm +module; +#include "header.h" +export module mymodule; + +//--- main.cpp +#include "header.h" +import mymodule; + +// CHECK-MESSAGES: header.h:19:5: warning: class 'B' can be made trivially destructible +B::~B() = default; // to-be-removed +// CHECK-MESSAGES: :[[@LINE-1]]:4: note: destructor definition is here +// CHECK-FIXES: // to-be-removed + +int main() { + X x; + B b; +} diff --git a/clang-tools-extra/test/lit.cfg.py b/clang-tools-extra/test/lit.cfg.py index c39ea29329674..a3901d6e3f5c9 100644 --- a/clang-tools-extra/test/lit.cfg.py +++ b/clang-tools-extra/test/lit.cfg.py @@ -49,8 +49,8 @@ # test_exec_root: The root path where tests should be run. config.test_exec_root = os.path.join(config.clang_tools_binary_dir, "test") -# Tools need the same environment setup as clang (we don't need clang itself). -llvm_config.clang_setup() +# Set up clang for use in tests. Makes %clang substitution available. +llvm_config.use_clang() if config.clang_tidy_staticanalyzer: config.available_features.add("static-analyzer") _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
