Skip to content

Conversation

@kadircet
Copy link
Member

These are available for all translations implicitly. We shouldn't report
explicit refs and enforce includes.

@kadircet
Copy link
Member Author

so this is the alternative to #123027. as discussed offline that change would break translation units that depend on a user-provided declaration for new/delete operators.

@llvmbot
Copy link
Member

llvmbot commented Jan 31, 2025

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

Author: kadir çetinkaya (kadircet)

Changes

These are available for all translations implicitly. We shouldn't report
explicit refs and enforce includes.


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

2 Files Affected:

  • (modified) clang-tools-extra/include-cleaner/lib/WalkAST.cpp (+15-1)
  • (modified) clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp (+46)
diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
index aae3eda519ffdc..e3686a29d4367d 100644
--- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
+++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
@@ -22,6 +22,7 @@
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/IdentifierTable.h"
+#include "clang/Basic/OperatorKinds.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
 #include "llvm/ADT/STLExtras.h"
@@ -32,6 +33,11 @@
 
 namespace clang::include_cleaner {
 namespace {
+bool isImplicitOperatorNewDelete(OverloadedOperatorKind OpKind) {
+  return OpKind == OO_New || OpKind == OO_Delete || OpKind == OO_Array_New ||
+         OpKind == OO_Array_Delete;
+}
+
 using DeclCallback =
     llvm::function_ref<void(SourceLocation, NamedDecl &, RefType)>;
 
@@ -158,7 +164,15 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> {
     // the container decl instead, which is preferred as it'll handle
     // aliases/exports properly.
     if (!FD->isCXXClassMember() && !llvm::isa<EnumConstantDecl>(FD)) {
-      report(DRE->getLocation(), FD);
+      // Global operator new/delete [] is available implicitly in every
+      // translation unit, even without including any explicit headers. So treat
+      // those as ambigious to not force inclusion in TUs that transitively
+      // depend on those.
+      RefType RT = isImplicitOperatorNewDelete(
+                       FD->getDeclName().getCXXOverloadedOperator())
+                       ? RefType::Ambiguous
+                       : RefType::Explicit;
+      report(DRE->getLocation(), FD, RT);
       return true;
     }
     // If the ref is without a qualifier, and is a member, ignore it. As it is
diff --git a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
index d2d137a0dfb42a..21797e1c6825ac 100644
--- a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
@@ -397,6 +397,52 @@ TEST_F(AnalyzeTest, SpellingIncludesWithSymlinks) {
   }
 }
 
+// Make sure that the references to implicit operator new/delete are reported as
+// ambigious.
+TEST_F(AnalyzeTest, ImplicitOperatorNewDelete) {
+  ExtraFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+  ExtraFS->addFile("header.h",
+                   /*ModificationTime=*/{},
+                   llvm::MemoryBuffer::getMemBufferCopy(guard(R"cpp(
+  void* operator new(decltype(sizeof(int)));
+  )cpp")));
+  ExtraFS->addFile("wrapper.h",
+                   /*ModificationTime=*/{},
+                   llvm::MemoryBuffer::getMemBufferCopy(guard(R"cpp(
+  #include "header.h"
+  )cpp")));
+
+  // Check that header.h is not reported as missing.
+  {
+    Inputs.Code = R"cpp(
+      #include "wrapper.h"
+      void bar() {
+        operator new(3);
+      })cpp";
+    TestAST AST(Inputs);
+    std::vector<Decl *> DeclsInTU;
+    for (auto *D : AST.context().getTranslationUnitDecl()->decls())
+      DeclsInTU.push_back(D);
+    auto Results = analyze(DeclsInTU, {}, PP.Includes, &PI, AST.preprocessor());
+    EXPECT_THAT(Results.Missing, testing::IsEmpty());
+  }
+
+  // Check that header.h is not reported as unused.
+  {
+    Inputs.Code = R"cpp(
+      #include "header.h"
+      void bar() {
+        operator new(3);
+      })cpp";
+    TestAST AST(Inputs);
+    std::vector<Decl *> DeclsInTU;
+    for (auto *D : AST.context().getTranslationUnitDecl()->decls())
+      DeclsInTU.push_back(D);
+    auto Results = analyze(DeclsInTU, {}, PP.Includes, &PI, AST.preprocessor());
+    EXPECT_THAT(Results.Unused, Not(testing::IsEmpty()));
+  }
+}
+
 TEST(FixIncludes, Basic) {
   llvm::StringRef Code = R"cpp(#include "d.h"
 #include "a.h"

…elete

These are available for all translations implicitly. We shouldn't report
explicit refs and enforce includes.
@kadircet kadircet merged commit fcb1234 into llvm:main Jan 31, 2025
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants