Skip to content

[include-cleaner] Turn new/delete usages to ambiguous references #105844

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

Merged
merged 1 commit into from
Aug 24, 2024

Conversation

kadircet
Copy link
Member

In practice most of these expressions just resolve to implicitly
provided operator new and standard says it's not necessary to include
<new> for that.
Hence this is resulting in a lot of churn in cases where inclusion of
<new> doesn't matter, and might even be undesired by the developer.

By switching to an ambiguous reference we try to find a middle ground
here, ensuring that we don't drop providers of operator new when the
developer explicitly listed them in the includes, and chose to believe
it's the implicitly provided operator new and don't insert an include
in other cases.

In practice most of these expressions just resolve to implicitly
provided `operator new` and standard says it's not necessary to include
`<new>` for that.
Hence this is resulting in a lot of churn in cases where inclusion of
`<new>` doesn't matter, and might even be undesired by the developer.

By switching to an ambiguous reference we try to find a middle ground
here, ensuring that we don't drop providers of `operator new` when the
developer explicitly listed them in the includes, and chose to believe
it's the implicitly provided `operator new` and don't insert an include
in other cases.
@llvmbot
Copy link
Member

llvmbot commented Aug 23, 2024

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

Author: kadir çetinkaya (kadircet)

Changes

In practice most of these expressions just resolve to implicitly
provided operator new and standard says it's not necessary to include
&lt;new&gt; for that.
Hence this is resulting in a lot of churn in cases where inclusion of
&lt;new&gt; doesn't matter, and might even be undesired by the developer.

By switching to an ambiguous reference we try to find a middle ground
here, ensuring that we don't drop providers of operator new when the
developer explicitly listed them in the includes, and chose to believe
it's the implicitly provided operator new and don't insert an include
in other cases.


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

2 Files Affected:

  • (modified) clang-tools-extra/include-cleaner/lib/WalkAST.cpp (+2-2)
  • (modified) clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp (+2-2)
diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
index a5ac3760a3be2a..598484d09712e5 100644
--- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
+++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
@@ -351,11 +351,11 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> {
   }
 
   bool VisitCXXNewExpr(CXXNewExpr *E) {
-    report(E->getExprLoc(), E->getOperatorNew());
+    report(E->getExprLoc(), E->getOperatorNew(), RefType::Ambiguous);
     return true;
   }
   bool VisitCXXDeleteExpr(CXXDeleteExpr *E) {
-    report(E->getExprLoc(), E->getOperatorDelete());
+    report(E->getExprLoc(), E->getOperatorDelete(), RefType::Ambiguous);
     return true;
   }
 };
diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
index b0a4473d4ad2b7..6c8eacbff1cea3 100644
--- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -557,9 +557,9 @@ TEST(WalkAST, FriendDecl) {
 }
 
 TEST(WalkAST, OperatorNewDelete) {
-  testWalk("void* $explicit^operator new(decltype(sizeof(int)), void*);",
+  testWalk("void* $ambiguous^operator new(decltype(sizeof(int)), void*);",
            "struct Bar { void foo() { Bar b; ^new (&b) Bar; } };");
-  testWalk("struct A { static void $explicit^operator delete(void*); };",
+  testWalk("struct A { static void $ambiguous^operator delete(void*); };",
            "void foo() { A a; ^delete &a; }");
 }
 } // namespace

Copy link
Contributor

@ilya-biryukov ilya-biryukov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@kadircet kadircet merged commit 74b538d into llvm:main Aug 24, 2024
10 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