llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Gareth Andrew Lloyd (Ignition) <details> <summary>Changes</summary> Add `unless(isImplicit())` to the matcher to exclude implicit destructors. With C++20 modules, when a class is seen through both a header include and a module import, its implicit destructor appears multiple times in the redeclaration chain. This causes one instance to have isFirstDecl()==false, which the check incorrectly interprets as an out-of-line defaulted destructor needing refactoring. This scenario only occurs with modules; without them, implicit destructors have a single declaration and always satisfy isFirstDecl(). The fix excludes implicit destructors entirely, which is correct since they are compiler-generated and not candidates for this refactoring. --- Full diff: https://github.com/llvm/llvm-project/pull/178471.diff 3 Files Affected: - (modified) clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp (+1-1) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5) - (added) clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible-module.cpp (+45) ``````````diff diff --git a/clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp b/clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp index 416c41d7acd66..47304727f54d7 100644 --- a/clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp @@ -34,7 +34,7 @@ void TriviallyDestructibleCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( cxxDestructorDecl( isDefaulted(), - unless(anyOf(isFirstDecl(), isVirtual(), + unless(anyOf(isImplicit(), isFirstDecl(), 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 754880bd1a381..11ca4857ee40d 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -166,6 +166,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/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..b07207fbe8750 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible-module.cpp @@ -0,0 +1,45 @@ +// REQUIRES: system-linux +// +// RUN: rm -rf %t && mkdir %t +// RUN: split-file %s %t +// +// Build the module first: +// RUN: clang++ -std=c++20 -x c++-module --precompile %t/mymodule.cppm -o %t/mymodule.pcm -I%t -fPIC +// +// Run clang-tidy on main.cpp which both includes the header and imports the module. +// This should NOT produce any warnings or fix conflicts for implicit destructors. +// The struct X has no user-declared destructor, only an implicit one. +// RUN: clang-tidy %t/main.cpp -checks='-*,performance-trivially-destructible' \ +// RUN: -- -std=c++20 -I%t -fmodule-file=mymodule=%t/mymodule.pcm 2>&1 \ +// RUN: | FileCheck %s --allow-empty +// +// CHECK-NOT: warning: +// CHECK-NOT: Fix conflicts with existing fix + +//--- header.h +#pragma once +#include <atomic> + +// A struct with an implicit destructor and an atomic member. +// With C++20 modules, when this header is included in both the global module +// fragment and directly in a file that imports the module, the implicit +// destructor can appear multiple times in the redeclaration chain. +// The check should NOT match implicit destructors. +struct X { + X() : a(0) {} + std::atomic<int> a; +}; + +//--- mymodule.cppm +module; +#include "header.h" +export module mymodule; + +//--- main.cpp +#include "header.h" +import mymodule; + +int main() { + X x; + return 0; +} `````````` </details> https://github.com/llvm/llvm-project/pull/178471 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
