[clang-tools-extra] 3402b77 - [include-cleaner] Only ignore builtins without a header
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 4 04:34:30 PDT 2023
Author: Kadir Cetinkaya
Date: 2023-04-04T13:28:56+02:00
New Revision: 3402b77db3ee83f0c576988631afa12cfba61285
URL: https://github.com/llvm/llvm-project/commit/3402b77db3ee83f0c576988631afa12cfba61285
DIFF: https://github.com/llvm/llvm-project/commit/3402b77db3ee83f0c576988631afa12cfba61285.diff
LOG: [include-cleaner] Only ignore builtins without a header
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.
Differential Revision: https://reviews.llvm.org/D147449
Added:
Modified:
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
Removed:
################################################################################
diff --git a/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp b/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
index e273d429cd8c3..1b705e1d52c2d 100644
--- a/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
+++ b/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 <string>
+#include <optional>
#include <utility>
namespace clang::include_cleaner {
@@ -106,37 +109,68 @@ hintedHeadersForStdHeaders(llvm::ArrayRef<tooling::stdlib::Header> Headers,
return Results;
}
-// Special-case the ambiguous standard library symbols (e.g. std::move) which
-// are not supported by the tooling stdlib lib.
-llvm::SmallVector<Hinted<Header>>
-headersForSpecialSymbol(const Symbol &S, const SourceManager &SM,
- 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<tooling::stdlib::Header>
+headerForAmbiguousStdSymbol(const NamedDecl *ND) {
+ if (!ND->isInStdNamespace())
return {};
-
- const auto *FD = S.declaration().getAsFunction();
+ const auto *FD = ND->getAsFunction();
if (!FD)
- return {};
-
- llvm::StringRef FName = symbolName(S);
- llvm::SmallVector<tooling::stdlib::Header> Headers;
+ return std::nullopt;
+ llvm::StringRef FName = symbolName(*ND);
if (FName == "move") {
if (FD->getNumParams() == 1)
// move(T&& t)
- Headers.push_back(*tooling::stdlib::Header::named("<utility>"));
+ return tooling::stdlib::Header::named("<utility>");
if (FD->getNumParams() == 3)
// move(InputIt first, InputIt last, OutputIt dest);
- Headers.push_back(*tooling::stdlib::Header::named("<algorithm>"));
+ return tooling::stdlib::Header::named("<algorithm>");
} else if (FName == "remove") {
if (FD->getNumParams() == 1)
// remove(const char*);
- Headers.push_back(*tooling::stdlib::Header::named("<cstdio>"));
+ return tooling::stdlib::Header::named("<cstdio>");
if (FD->getNumParams() == 3)
// remove(ForwardIt first, ForwardIt last, const T& value);
- Headers.push_back(*tooling::stdlib::Header::named("<algorithm>"));
+ return tooling::stdlib::Header::named("<algorithm>");
}
- return applyHints(hintedHeadersForStdHeaders(Headers, SM, PI),
- Hints::CompleteSymbol);
+ return std::nullopt;
+}
+
+// Special-case symbols without proper locations, like the ambiguous standard
+// library symbols (e.g. std::move) or builtin declarations.
+std::optional<llvm::SmallVector<Hinted<Header>>>
+headersForSpecialSymbol(const Symbol &S, const SourceManager &SM,
+ const PragmaIncludes *PI) {
+ // Our special casing logic only deals with decls, so bail out early for
+ // macros.
+ if (S.kind() != Symbol::Declaration)
+ return std::nullopt;
+ const auto *ND = llvm::cast<NamedDecl>(&S.declaration());
+ // We map based on names, so again bail out early if there are no names.
+ if (!ND)
+ return std::nullopt;
+ auto *II = ND->getIdentifier();
+ if (!II)
+ return std::nullopt;
+
+ // Check first for symbols that are part of our stdlib mapping. As we have
+ // header names for those.
+ if (auto Header = headerForAmbiguousStdSymbol(ND)) {
+ return applyHints(hintedHeadersForStdHeaders({*Header}, SM, PI),
+ Hints::CompleteSymbol);
+ }
+
+ // Now check for builtin symbols, we shouldn't suggest any headers for ones
+ // without any headers.
+ if (auto ID = II->getBuiltinID()) {
+ const char *BuiltinHeader =
+ ND->getASTContext().BuiltinInfo.getHeaderName(ID);
+ if (!BuiltinHeader)
+ return llvm::SmallVector<Hinted<Header>>{};
+ // FIXME: Use the header mapping for builtins with a known header.
+ }
+ return std::nullopt;
}
} // namespace
@@ -187,11 +221,13 @@ llvm::SmallVector<Header> headersForSymbol(const Symbol &S,
// Get headers for all the locations providing Symbol. Same header can be
// reached through
diff erent traversals, deduplicate those into a single
// Header by merging their hints.
- llvm::SmallVector<Hinted<Header>> Headers =
- headersForSpecialSymbol(S, SM, PI);
- if (Headers.empty())
+ llvm::SmallVector<Hinted<Header>> Headers;
+ if (auto SpecialHeaders = headersForSpecialSymbol(S, SM, PI)) {
+ Headers = std::move(*SpecialHeaders);
+ } else {
for (auto &Loc : locateSymbol(S))
Headers.append(applyHints(findHeaders(Loc, SM, PI), Loc.Hint));
+ }
// If two Headers probably refer to the same file (e.g. Verbatim(foo.h) and
// Physical(/path/to/foo.h), we won't deduplicate them or merge their hints
llvm::stable_sort(
diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
index ab1476ea09972..f702460ccbb22 100644
--- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
+++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
@@ -35,9 +35,6 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> {
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<NamedDecl>(ND->getCanonicalDecl()), RT);
}
diff --git a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
index be2fc142eec61..dc649b1392294 100644
--- a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
@@ -93,12 +93,14 @@ TEST_F(WalkUsedTest, Basic) {
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 @@ TEST_F(WalkUsedTest, Basic) {
auto PublicFile = Header("\"path/public.h\"");
auto MainFile = Header(SM.getFileEntryForID(SM.getMainFileID()));
auto VectorSTL = Header(*tooling::stdlib::Header::named("<vector>"));
+ auto UtilitySTL = Header(*tooling::stdlib::Header::named("<utility>"));
EXPECT_THAT(
offsetToProviders(AST, SM),
UnorderedElementsAre(
@@ -122,8 +125,9 @@ TEST_F(WalkUsedTest, Basic) {
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) {
diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
index ac2085d82c1d8..59e0ff117e88c 100644
--- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -327,11 +327,5 @@ TEST(WalkAST, Enums) {
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
More information about the cfe-commits
mailing list