[PATCH] D147449: [include-cleaner] Only ignore builtins without a header

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3402b77db3ee: [include-cleaner] Only ignore builtins without 
a header (authored by kadircet).

Changed prior to commit:
  https://reviews.llvm.org/D147449?vs=510516=510746#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147449/new/

https://reviews.llvm.org/D147449

Files:
  clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
  clang-tools-extra/include-cleaner/lib/WalkAST.cpp
  clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
  clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp

Index: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
===
--- clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -327,11 +327,5 @@
   testWalk("enum class E : int {};", "enum class ^E : int ;");
 }
 
-TEST(WalkAST, BuiltinSymbols) {
-  testWalk(R"cpp(
-extern "C" int __builtin_popcount(unsigned int) noexcept;
-  )cpp", "int x = ^__builtin_popcount(1);");
-}
-
 } // namespace
 } // namespace clang::include_cleaner
Index: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
===
--- clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
+++ clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
@@ -93,12 +93,14 @@
   void $bar^bar($private^Private $p^p) {
 $foo^foo();
 std::$vector^vector $vconstructor^$v^v;
+$builtin^__builtin_popcount(1);
+std::$move^move(3);
   }
   )cpp");
   Inputs.Code = Code.code();
   Inputs.ExtraFiles["header.h"] = guard(R"cpp(
   void foo();
-  namespace std { class vector {}; }
+  namespace std { class vector {}; int&& move(int&&); }
   )cpp");
   Inputs.ExtraFiles["private.h"] = guard(R"cpp(
 // IWYU pragma: private, include "path/public.h"
@@ -112,6 +114,7 @@
   auto PublicFile = Header("\"path/public.h\"");
   auto MainFile = Header(SM.getFileEntryForID(SM.getMainFileID()));
   auto VectorSTL = Header(*tooling::stdlib::Header::named(""));
+  auto UtilitySTL = Header(*tooling::stdlib::Header::named(""));
   EXPECT_THAT(
   offsetToProviders(AST, SM),
   UnorderedElementsAre(
@@ -122,8 +125,9 @@
   Pair(Code.point("foo"), UnorderedElementsAre(HeaderFile)),
   Pair(Code.point("vector"), UnorderedElementsAre(VectorSTL)),
   Pair(Code.point("vconstructor"), UnorderedElementsAre(VectorSTL)),
-  Pair(Code.point("v"), UnorderedElementsAre(MainFile))
-  ));
+  Pair(Code.point("v"), UnorderedElementsAre(MainFile)),
+  Pair(Code.point("builtin"), testing::IsEmpty()),
+  Pair(Code.point("move"), UnorderedElementsAre(UtilitySTL;
 }
 
 TEST_F(WalkUsedTest, MultipleProviders) {
Index: clang-tools-extra/include-cleaner/lib/WalkAST.cpp
===
--- clang-tools-extra/include-cleaner/lib/WalkAST.cpp
+++ clang-tools-extra/include-cleaner/lib/WalkAST.cpp
@@ -35,9 +35,6 @@
   RefType RT = RefType::Explicit) {
 if (!ND || Loc.isInvalid())
   return;
-// Don't report builtin symbols.
-if (const auto *II = ND->getIdentifier(); II && II->getBuiltinID() > 0)
-  return;
 Callback(Loc, *cast(ND->getCanonicalDecl()), RT);
   }
 
Index: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
===
--- clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
+++ clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
@@ -10,18 +10,21 @@
 #include "TypesInternal.h"
 #include "clang-include-cleaner/Record.h"
 #include "clang-include-cleaner/Types.h"
+#include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
+#include "clang/Basic/Builtins.h"
 #include "clang/Basic/FileEntry.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Inclusions/StandardLibrary.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
-#include 
+#include 
 #include 
 
 namespace clang::include_cleaner {
@@ -106,37 +109,68 @@
   return Results;
 }
 
-// Special-case the ambiguous standard library symbols (e.g. std::move) which
-// are not supported by the tooling stdlib lib.
-llvm::SmallVector>
-headersForSpecialSymbol(const Symbol , const SourceManager ,
-const PragmaIncludes *PI) {
-  if (S.kind() != Symbol::Declaration || !S.declaration().isInStdNamespace())
+// Symbol to header mapping for std::move and std::remove, based on number of
+// parameters.
+std::optional
+headerForAmbiguousStdSymbol(const NamedDecl *ND) {
+  if (!ND->isInStdNamespace())
  

[PATCH] D147449: [include-cleaner] Only ignore builtins without a header

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

Thanks. I was surprised to see that some standard library symbols are treated 
as builtin symbols.

I think the current approach is better (e.g. clangd's hover on 
`__builtin_popcount()` will not give some arbitrary header providers).




Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:169
+ND->getASTContext().BuiltinInfo.getHeaderName(ID);
+// FIXME: Use the header mapping for builtins with a known header.
+if (!BuiltinHeader)

I think it would be clearer if we move this FIXME after the following if 
branch. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147449/new/

https://reviews.llvm.org/D147449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147449: [include-cleaner] Only ignore builtins without a header

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: hokein.
Herald added a project: All.
kadircet requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Certain standard library functions (e.g. std::move) are also implemented
as builtins. This patch moves filtering logic to the symbol->header
mapping phase to rather generate these references without any providers
only when we don't have a mapping.
That way we can also map them to header names mentioned in the builtin
mappings.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147449

Files:
  clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
  clang-tools-extra/include-cleaner/lib/WalkAST.cpp
  clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
  clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp

Index: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
===
--- clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -327,11 +327,5 @@
   testWalk("enum class E : int {};", "enum class ^E : int ;");
 }
 
-TEST(WalkAST, BuiltinSymbols) {
-  testWalk(R"cpp(
-extern "C" int __builtin_popcount(unsigned int) noexcept;
-  )cpp", "int x = ^__builtin_popcount(1);");
-}
-
 } // namespace
 } // namespace clang::include_cleaner
Index: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
===
--- clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
+++ clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
@@ -93,12 +93,14 @@
   void $bar^bar($private^Private $p^p) {
 $foo^foo();
 std::$vector^vector $vconstructor^$v^v;
+$builtin^__builtin_popcount(1);
+std::$move^move(3);
   }
   )cpp");
   Inputs.Code = Code.code();
   Inputs.ExtraFiles["header.h"] = guard(R"cpp(
   void foo();
-  namespace std { class vector {}; }
+  namespace std { class vector {}; int&& move(int&&); }
   )cpp");
   Inputs.ExtraFiles["private.h"] = guard(R"cpp(
 // IWYU pragma: private, include "path/public.h"
@@ -112,6 +114,7 @@
   auto PublicFile = Header("\"path/public.h\"");
   auto MainFile = Header(SM.getFileEntryForID(SM.getMainFileID()));
   auto VectorSTL = Header(*tooling::stdlib::Header::named(""));
+  auto UtilitySTL = Header(*tooling::stdlib::Header::named(""));
   EXPECT_THAT(
   offsetToProviders(AST, SM),
   UnorderedElementsAre(
@@ -122,8 +125,9 @@
   Pair(Code.point("foo"), UnorderedElementsAre(HeaderFile)),
   Pair(Code.point("vector"), UnorderedElementsAre(VectorSTL)),
   Pair(Code.point("vconstructor"), UnorderedElementsAre(VectorSTL)),
-  Pair(Code.point("v"), UnorderedElementsAre(MainFile))
-  ));
+  Pair(Code.point("v"), UnorderedElementsAre(MainFile)),
+  Pair(Code.point("builtin"), testing::IsEmpty()),
+  Pair(Code.point("move"), UnorderedElementsAre(UtilitySTL;
 }
 
 TEST_F(WalkUsedTest, MultipleProviders) {
Index: clang-tools-extra/include-cleaner/lib/WalkAST.cpp
===
--- clang-tools-extra/include-cleaner/lib/WalkAST.cpp
+++ clang-tools-extra/include-cleaner/lib/WalkAST.cpp
@@ -35,9 +35,6 @@
   RefType RT = RefType::Explicit) {
 if (!ND || Loc.isInvalid())
   return;
-// Don't report builtin symbols.
-if (const auto *II = ND->getIdentifier(); II && II->getBuiltinID() > 0)
-  return;
 Callback(Loc, *cast(ND->getCanonicalDecl()), RT);
   }
 
Index: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
===
--- clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
+++ clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
@@ -10,18 +10,21 @@
 #include "TypesInternal.h"
 #include "clang-include-cleaner/Record.h"
 #include "clang-include-cleaner/Types.h"
+#include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
+#include "clang/Basic/Builtins.h"
 #include "clang/Basic/FileEntry.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Inclusions/StandardLibrary.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
-#include 
+#include 
 #include 
 
 namespace clang::include_cleaner {
@@ -106,37 +109,68 @@
   return Results;
 }
 
-// Special-case the ambiguous standard library symbols (e.g. std::move) which
-// are not supported by the tooling stdlib lib.
-llvm::SmallVector>
-headersForSpecialSymbol(const Symbol , const SourceManager ,
-const PragmaIncludes *PI) {
-  if (S.kind() != Symbol::Declaration ||