Skip to content

[include-cleaner] Add special mappings for operator new/delete #123027

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

kadircet
Copy link
Member

Our stdlib mappings are based on names, hence they can't handle special
symbols like oprators.

Global operator new/delete show up often enough in practice to create
some user frustration, so we map these to .

Our stdlib mappings are based on names, hence they can't handle special
symbols like oprators.

Global operator new/delete show up often enough in practice to create
some user frustration, so we map these to <new>.
@llvmbot
Copy link
Member

llvmbot commented Jan 15, 2025

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

Author: kadir çetinkaya (kadircet)

Changes

Our stdlib mappings are based on names, hence they can't handle special
symbols like oprators.

Global operator new/delete show up often enough in practice to create
some user frustration, so we map these to <new>.


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

2 Files Affected:

  • (modified) clang-tools-extra/include-cleaner/lib/FindHeaders.cpp (+16-1)
  • (modified) clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp (+45)
diff --git a/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp b/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
index 7b28d1c252d715..e6a642ae8ed48a 100644
--- a/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
+++ b/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
@@ -16,6 +16,7 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/FileEntry.h"
+#include "clang/Basic/OperatorKinds.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Inclusions/StandardLibrary.h"
@@ -157,8 +158,22 @@ headersForSpecialSymbol(const Symbol &S, const SourceManager &SM,
   if (!ND)
     return std::nullopt;
   auto *II = ND->getIdentifier();
-  if (!II)
+  if (!II) {
+    // Special case global operator new/delete, these show up often enough in
+    // practice and stdlib mappings can't work with them as they're symbol-name
+    // based.
+    if (ND->getDeclContext()->isTranslationUnit()) {
+      switch (ND->getDeclName().getCXXOverloadedOperator()) {
+      case OverloadedOperatorKind::OO_New:
+      case OverloadedOperatorKind::OO_Delete:
+        return hintedHeadersForStdHeaders(
+            {tooling::stdlib::Header::named("<new>").value()}, SM, PI);
+      default:
+        break;
+      }
+    }
     return std::nullopt;
+  }
 
   // Check first for symbols that are part of our stdlib mapping. As we have
   // header names for those.
diff --git a/clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp b/clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp
index 84e02e1d0d621b..d5b0a61ed1dfcb 100644
--- a/clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp
@@ -11,11 +11,13 @@
 #include "clang-include-cleaner/Analysis.h"
 #include "clang-include-cleaner/Record.h"
 #include "clang-include-cleaner/Types.h"
+#include "clang/AST/DynamicRecursiveASTVisitor.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/FileEntry.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/LLVM.h"
+#include "clang/Basic/OperatorKinds.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Testing/TestAST.h"
 #include "clang/Tooling/Inclusions/StandardLibrary.h"
@@ -617,6 +619,49 @@ TEST_F(HeadersForSymbolTest, AmbiguousStdSymbolsUsingShadow) {
                   Header(*tooling::stdlib::Header::named("<cstdio>"))));
 }
 
+TEST_F(HeadersForSymbolTest, GlobalOperatorNewDelete) {
+  Inputs.Code = R"cpp(
+    void k() {
+      int *x;
+      // make sure operator new/delete are part of TU.
+      x = static_cast<int*>(::operator new(sizeof(int)));
+      ::operator delete(x);
+    }
+  )cpp";
+  buildAST();
+
+  // Find global new/delete operators.
+  struct Visitor : public DynamicRecursiveASTVisitor {
+    const NamedDecl *New = nullptr;
+    const NamedDecl *Delete = nullptr;
+    bool VisitNamedDecl(NamedDecl *ND) override {
+      if (!ND->getDeclContext()->isTranslationUnit())
+        return true;
+      switch (ND->getDeclName().getCXXOverloadedOperator()) {
+      case OO_New:
+        New = ND;
+        break;
+      case OO_Delete:
+        Delete = ND;
+        break;
+      default:
+        break;
+      }
+      return true;
+    }
+  };
+  Visitor V;
+  V.ShouldVisitImplicitCode = true;
+  V.TraverseDecl(AST->context().getTranslationUnitDecl());
+  ASSERT_TRUE(V.New) << "Couldn't find global new!";
+  ASSERT_TRUE(V.Delete) << "Couldn't find global delete!";
+  EXPECT_THAT(headersForSymbol(*V.New, AST->sourceManager(), &PI),
+              UnorderedElementsAre(
+                  Header(*tooling::stdlib::Header::named("<new>"))));
+  EXPECT_THAT(headersForSymbol(*V.Delete, AST->sourceManager(), &PI),
+              UnorderedElementsAre(
+                  Header(*tooling::stdlib::Header::named("<new>"))));
+}
 
 TEST_F(HeadersForSymbolTest, StandardHeaders) {
   Inputs.Code = R"cpp(

Copy link

github-actions bot commented Jan 15, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@kadircet
Copy link
Member Author

abandoning in favor of #125199

@kadircet kadircet closed this Jan 31, 2025
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.

2 participants