https://github.com/Ignition updated https://github.com/llvm/llvm-project/pull/178471
>From 6fa24dab52fac03118696219dfd18ca124f03b4d Mon Sep 17 00:00:00 2001 From: Gareth Lloyd <[email protected]> Date: Wed, 28 Jan 2026 17:10:30 +0000 Subject: [PATCH] [clang-tidy] Fix performance-trivially-destructible with C++20 modules 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 --- .../TriviallyDestructibleCheck.cpp | 10 ++- clang-tools-extra/docs/ReleaseNotes.rst | 5 ++ clang-tools-extra/test/CMakeLists.txt | 3 + .../trivially-destructible-module.cpp | 61 +++++++++++++++++++ clang-tools-extra/test/lit.cfg.py | 4 +- 5 files changed, 78 insertions(+), 5 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible-module.cpp 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 0ad69f5fdc5aa..0709d4d716356 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -194,6 +194,11 @@ Changes in existing checks <clang-tidy/checks/performance/move-const-arg>` check by avoiding false positives on trivially copyable types with a non-public copy constructor. +- 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-enum-initial-value <clang-tidy/checks/readability/enum-initial-value>` check: the warning message now uses separate note diagnostics for each uninitialized enumerator, making diff --git a/clang-tools-extra/test/CMakeLists.txt b/clang-tools-extra/test/CMakeLists.txt index 78447e7a00db8..1182fb4e7c113 100644 --- a/clang-tools-extra/test/CMakeLists.txt +++ b/clang-tools-extra/test/CMakeLists.txt @@ -51,6 +51,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
