[include-cleaner] Respect langopts when analyzing macro names #123634
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clangd Author: kadir çetinkaya (kadircet) ChangesFull diff: https://github.com/llvm/llvm-project/pull/123634.diff 11 Files Affected:
diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp
index 5e136d0e76ece7..3ab3d890305204 100644
--- a/clang-tools-extra/clangd/Hover.cpp
+++ b/clang-tools-extra/clangd/Hover.cpp
@@ -1193,12 +1193,13 @@ void maybeAddSymbolProviders(ParsedAST &AST, HoverInfo &HI,
include_cleaner::Symbol Sym) {
trace::Span Tracer("Hover::maybeAddSymbolProviders");
- const SourceManager &SM = AST.getSourceManager();
llvm::SmallVector<include_cleaner::Header> RankedProviders =
- include_cleaner::headersForSymbol(Sym, SM, &AST.getPragmaIncludes());
+ include_cleaner::headersForSymbol(Sym, AST.getPreprocessor(),
+ &AST.getPragmaIncludes());
if (RankedProviders.empty())
return;
+ const SourceManager &SM = AST.getSourceManager();
std::string Result;
include_cleaner::Includes ConvertedIncludes = convertIncludes(AST);
for (const auto &P : RankedProviders) {
diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index 6d0af20e31260c..1de7faf81746ee 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -888,7 +888,7 @@ void SymbolCollector::setIncludeLocation(const Symbol &S, SourceLocation DefLoc,
// might run while parsing, rather than at the end of a translation unit.
// Hence we see more and more redecls over time.
SymbolProviders[S.ID] =
- include_cleaner::headersForSymbol(Sym, SM, Opts.PragmaIncludes);
+ include_cleaner::headersForSymbol(Sym, *PP, Opts.PragmaIncludes);
}
llvm::StringRef getStdHeader(const Symbol *S, const LangOptions &LangOpts) {
diff --git a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h
index 46ca3c9d080740..c3241763237d14 100644
--- a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h
+++ b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h
@@ -90,7 +90,7 @@ std::string fixIncludes(const AnalysisResults &Results,
/// Returned headers are sorted by relevance, first element is the most
/// likely provider for the symbol.
llvm::SmallVector<Header> headersForSymbol(const Symbol &S,
- const SourceManager &SM,
+ const Preprocessor &PP,
const PragmaIncludes *PI);
} // namespace include_cleaner
} // namespace clang
diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp b/clang-tools-extra/include-cleaner/lib/Analysis.cpp
index e3a4834cb19aeb..a1781f4e24f2e8 100644
--- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp
+++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp
@@ -64,7 +64,7 @@ void walkUsed(llvm::ArrayRef<Decl *> ASTRoots,
// FIXME: Most of the work done here is repetitive. It might be useful to
// have a cache/batching.
SymbolReference SymRef{ND, Loc, RT};
- return CB(SymRef, headersForSymbol(ND, SM, PI));
+ return CB(SymRef, headersForSymbol(ND, PP, PI));
});
}
for (const SymbolReference &MacroRef : MacroRefs) {
@@ -72,7 +72,7 @@ void walkUsed(llvm::ArrayRef<Decl *> ASTRoots,
if (!SM.isWrittenInMainFile(SM.getSpellingLoc(MacroRef.RefLocation)) ||
shouldIgnoreMacroReference(PP, MacroRef.Target.macro()))
continue;
- CB(MacroRef, headersForSymbol(MacroRef.Target, SM, PI));
+ CB(MacroRef, headersForSymbol(MacroRef.Target, PP, PI));
}
}
diff --git a/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h b/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h
index cd796c2da7b805..7d170fd15014d2 100644
--- a/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h
+++ b/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h
@@ -25,6 +25,8 @@
#include "clang-include-cleaner/Analysis.h"
#include "clang-include-cleaner/Record.h"
#include "clang-include-cleaner/Types.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Lex/Preprocessor.h"
#include "llvm/ADT/STLFunctionalExtras.h"
#include <vector>
@@ -57,13 +59,14 @@ llvm::SmallVector<Hinted<Header>> findHeaders(const SymbolLocation &Loc,
const PragmaIncludes *PI);
/// A set of locations that provides the declaration.
-std::vector<Hinted<SymbolLocation>> locateSymbol(const Symbol &S);
+std::vector<Hinted<SymbolLocation>> locateSymbol(const Symbol &S,
+ const LangOptions &LO);
/// Write an HTML summary of the analysis to the given stream.
void writeHTMLReport(FileID File, const Includes &,
llvm::ArrayRef<Decl *> Roots,
llvm::ArrayRef<SymbolReference> MacroRefs, ASTContext &Ctx,
- const HeaderSearch &HS, PragmaIncludes *PI,
+ const Preprocessor &PP, PragmaIncludes *PI,
llvm::raw_ostream &OS);
} // namespace include_cleaner
diff --git a/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp b/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
index 7b28d1c252d715..b96d9a70728c23 100644
--- a/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
+++ b/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
@@ -18,6 +18,7 @@
#include "clang/Basic/FileEntry.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Preprocessor.h"
#include "clang/Tooling/Inclusions/StandardLibrary.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/STLExtras.h"
@@ -239,8 +240,9 @@ llvm::SmallVector<Hinted<Header>> findHeaders(const SymbolLocation &Loc,
}
llvm::SmallVector<Header> headersForSymbol(const Symbol &S,
- const SourceManager &SM,
+ const Preprocessor &PP,
const PragmaIncludes *PI) {
+ const auto &SM = PP.getSourceManager();
// Get headers for all the locations providing Symbol. Same header can be
// reached through different traversals, deduplicate those into a single
// Header by merging their hints.
@@ -248,7 +250,7 @@ llvm::SmallVector<Header> headersForSymbol(const Symbol &S,
if (auto SpecialHeaders = headersForSpecialSymbol(S, SM, PI)) {
Headers = std::move(*SpecialHeaders);
} else {
- for (auto &Loc : locateSymbol(S))
+ for (auto &Loc : locateSymbol(S, PP.getLangOpts()))
Headers.append(applyHints(findHeaders(Loc, SM, PI), Loc.Hint));
}
// If two Headers probably refer to the same file (e.g. Verbatim(foo.h) and
diff --git a/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp b/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp
index bbe8bc230c6e20..92c7c554ca50c9 100644
--- a/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp
+++ b/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp
@@ -21,6 +21,7 @@
#include "clang/Basic/SourceManager.h"
#include "clang/Lex/HeaderSearch.h"
#include "clang/Lex/Lexer.h"
+#include "clang/Lex/Preprocessor.h"
#include "clang/Tooling/Inclusions/StandardLibrary.h"
#include "llvm/Support/ScopedPrinter.h"
#include "llvm/Support/raw_ostream.h"
@@ -135,7 +136,7 @@ class Reporter {
llvm::raw_ostream &OS;
const ASTContext &Ctx;
const SourceManager &SM;
- const HeaderSearch &HS;
+ const Preprocessor &PP;
const include_cleaner::Includes &Includes;
const PragmaIncludes *PI;
FileID MainFile;
@@ -170,9 +171,9 @@ class Reporter {
void fillTarget(Ref &R) {
// Duplicates logic from walkUsed(), which doesn't expose SymbolLocations.
- for (auto &Loc : locateSymbol(R.Sym))
+ for (auto &Loc : locateSymbol(R.Sym, Ctx.getLangOpts()))
R.Locations.push_back(Loc);
- R.Headers = headersForSymbol(R.Sym, SM, PI);
+ R.Headers = headersForSymbol(R.Sym, PP, PI);
for (const auto &H : R.Headers) {
R.Includes.append(Includes.match(H));
@@ -189,14 +190,15 @@ class Reporter {
R.Includes.end());
if (!R.Headers.empty())
- R.Insert = spellHeader({R.Headers.front(), HS, MainFE});
+ R.Insert =
+ spellHeader({R.Headers.front(), PP.getHeaderSearchInfo(), MainFE});
}
public:
- Reporter(llvm::raw_ostream &OS, ASTContext &Ctx, const HeaderSearch &HS,
+ Reporter(llvm::raw_ostream &OS, ASTContext &Ctx, const Preprocessor &PP,
const include_cleaner::Includes &Includes, const PragmaIncludes *PI,
FileID MainFile)
- : OS(OS), Ctx(Ctx), SM(Ctx.getSourceManager()), HS(HS),
+ : OS(OS), Ctx(Ctx), SM(Ctx.getSourceManager()), PP(PP),
Includes(Includes), PI(PI), MainFile(MainFile),
MainFE(SM.getFileEntryForID(MainFile)) {}
@@ -498,9 +500,9 @@ class Reporter {
void writeHTMLReport(FileID File, const include_cleaner::Includes &Includes,
llvm::ArrayRef<Decl *> Roots,
llvm::ArrayRef<SymbolReference> MacroRefs, ASTContext &Ctx,
- const HeaderSearch &HS, PragmaIncludes *PI,
+ const Preprocessor &PP, PragmaIncludes *PI,
llvm::raw_ostream &OS) {
- Reporter R(OS, Ctx, HS, Includes, PI, File);
+ Reporter R(OS, Ctx, PP, Includes, PI, File);
const auto& SM = Ctx.getSourceManager();
for (Decl *Root : Roots)
walkAST(*Root, [&](SourceLocation Loc, const NamedDecl &D, RefType T) {
diff --git a/clang-tools-extra/include-cleaner/lib/LocateSymbol.cpp b/clang-tools-extra/include-cleaner/lib/LocateSymbol.cpp
index 78e783a62eb27f..b7433305152f95 100644
--- a/clang-tools-extra/include-cleaner/lib/LocateSymbol.cpp
+++ b/clang-tools-extra/include-cleaner/lib/LocateSymbol.cpp
@@ -54,20 +54,24 @@ std::vector<Hinted<SymbolLocation>> locateDecl(const Decl &D) {
return Result;
}
-std::vector<Hinted<SymbolLocation>> locateMacro(const Macro &M) {
+std::vector<Hinted<SymbolLocation>> locateMacro(const Macro &M,
+ const tooling::stdlib::Lang L) {
// FIXME: Should we also provide physical locations?
- if (auto SS = tooling::stdlib::Symbol::named("", M.Name->getName()))
+ if (auto SS = tooling::stdlib::Symbol::named("", M.Name->getName(), L))
return {{*SS, Hints::CompleteSymbol}};
return {{M.Definition, Hints::CompleteSymbol}};
}
} // namespace
-std::vector<Hinted<SymbolLocation>> locateSymbol(const Symbol &S) {
+std::vector<Hinted<SymbolLocation>> locateSymbol(const Symbol &S,
+ const LangOptions &LO) {
+ const auto L = !LO.CPlusPlus && LO.C99 ? tooling::stdlib::Lang::C
+ : tooling::stdlib::Lang::CXX;
switch (S.kind()) {
case Symbol::Declaration:
return locateDecl(S.declaration());
case Symbol::Macro:
- return locateMacro(S.macro());
+ return locateMacro(S.macro(), L);
}
llvm_unreachable("Unknown Symbol::Kind enum");
}
diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
index f85dbc0e0c31f2..1d9458ffc4d327 100644
--- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
+++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
@@ -216,10 +216,9 @@ class Action : public clang::ASTFrontendAction {
++Errors;
return;
}
- writeHTMLReport(
- AST.Ctx->getSourceManager().getMainFileID(), PP.Includes, AST.Roots,
- PP.MacroReferences, *AST.Ctx,
- getCompilerInstance().getPreprocessor().getHeaderSearchInfo(), &PI, OS);
+ writeHTMLReport(AST.Ctx->getSourceManager().getMainFileID(), PP.Includes,
+ AST.Roots, PP.MacroReferences, *AST.Ctx,
+ getCompilerInstance().getPreprocessor(), &PI, OS);
}
};
class ActionFactory : public tooling::FrontendActionFactory {
diff --git a/clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp b/clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp
index 84e02e1d0d621b..0ac243937e6e4f 100644
--- a/clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp
@@ -306,7 +306,7 @@ class HeadersForSymbolTest : public FindHeadersTest {
if (!V.Out)
ADD_FAILURE() << "Couldn't find any decls named " << Name << ".";
assert(V.Out);
- return headersForSymbol(*V.Out, AST->sourceManager(), &PI);
+ return headersForSymbol(*V.Out, AST->preprocessor(), &PI);
}
llvm::SmallVector<Header> headersForFoo() { return headersFor("foo"); }
};
@@ -611,13 +611,12 @@ TEST_F(HeadersForSymbolTest, AmbiguousStdSymbolsUsingShadow) {
Visitor V;
V.TraverseDecl(AST->context().getTranslationUnitDecl());
ASSERT_TRUE(V.Out) << "Couldn't find a DeclRefExpr!";
- EXPECT_THAT(headersForSymbol(*(V.Out->getFoundDecl()),
- AST->sourceManager(), &PI),
- UnorderedElementsAre(
- Header(*tooling::stdlib::Header::named("<cstdio>"))));
+ EXPECT_THAT(
+ headersForSymbol(*(V.Out->getFoundDecl()), AST->preprocessor(), &PI),
+ UnorderedElementsAre(
+ Header(*tooling::stdlib::Header::named("<cstdio>"))));
}
-
TEST_F(HeadersForSymbolTest, StandardHeaders) {
Inputs.Code = R"cpp(
#include "stdlib_internal.h"
@@ -636,6 +635,30 @@ TEST_F(HeadersForSymbolTest, StandardHeaders) {
tooling::stdlib::Header::named("<assert.h>")));
}
+TEST_F(HeadersForSymbolTest, StdlibLangForMacros) {
+ Inputs.Code = R"cpp(
+ #define EOF 0
+ void foo() { EOF; }
+ )cpp";
+ {
+ buildAST();
+ const Macro Eof{AST->preprocessor().getIdentifierInfo("EOF"), {}};
+ EXPECT_THAT(
+ headersForSymbol(Eof, AST->preprocessor(), nullptr),
+ UnorderedElementsAre(tooling::stdlib::Header::named("<cstdio>"),
+ tooling::stdlib::Header::named("<stdio.h>")));
+ }
+
+ {
+ Inputs.ExtraArgs.push_back("-xc");
+ buildAST();
+ const Macro Eof{AST->preprocessor().getIdentifierInfo("EOF"), {}};
+ EXPECT_THAT(headersForSymbol(Eof, AST->preprocessor(), nullptr),
+ UnorderedElementsAre(tooling::stdlib::Header::named(
+ "<stdio.h>", tooling::stdlib::Lang::C)));
+ }
+}
+
TEST_F(HeadersForSymbolTest, ExporterNoNameMatch) {
Inputs.Code = R"cpp(
#include "exporter/foo.h"
diff --git a/clang-tools-extra/include-cleaner/unittests/LocateSymbolTest.cpp b/clang-tools-extra/include-cleaner/unittests/LocateSymbolTest.cpp
index 756757cfd0f09d..1e7baf142a75ac 100644
--- a/clang-tools-extra/include-cleaner/unittests/LocateSymbolTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/LocateSymbolTest.cpp
@@ -11,6 +11,7 @@
#include "clang/AST/Decl.h"
#include "clang/AST/DeclBase.h"
#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Basic/LangOptions.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Testing/TestAST.h"
@@ -96,6 +97,8 @@ struct LocateExample {
Results.emplace_back(SM.getComposedLoc(FID, Offset));
return Results;
}
+
+ const LangOptions &langOpts() { return AST.preprocessor().getLangOpts(); }
};
TEST(LocateSymbol, Decl) {
@@ -110,7 +113,7 @@ TEST(LocateSymbol, Decl) {
for (auto &Case : Cases) {
SCOPED_TRACE(Case);
LocateExample Test(Case);
- EXPECT_THAT(locateSymbol(Test.findDecl("foo")),
+ EXPECT_THAT(locateSymbol(Test.findDecl("foo"), Test.langOpts()),
ElementsAreArray(Test.points()));
}
}
@@ -119,12 +122,12 @@ TEST(LocateSymbol, Stdlib) {
{
LocateExample Test("namespace std { struct vector; }");
EXPECT_THAT(
- locateSymbol(Test.findDecl("vector")),
+ locateSymbol(Test.findDecl("vector"), Test.langOpts()),
ElementsAre(*tooling::stdlib::Symbol::named("std::", "vector")));
}
{
LocateExample Test("#define assert(x)\nvoid foo() { assert(true); }");
- EXPECT_THAT(locateSymbol(Test.findMacro("assert")),
+ EXPECT_THAT(locateSymbol(Test.findMacro("assert"), Test.langOpts()),
ElementsAre(*tooling::stdlib::Symbol::named("", "assert")));
}
}
@@ -132,7 +135,7 @@ TEST(LocateSymbol, Stdlib) {
TEST(LocateSymbol, Macros) {
// Make sure we preserve the last one.
LocateExample Test("#define FOO\n#undef FOO\n#define ^FOO");
- EXPECT_THAT(locateSymbol(Test.findMacro("FOO")),
+ EXPECT_THAT(locateSymbol(Test.findMacro("FOO"), Test.langOpts()),
ElementsAreArray(Test.points()));
}
@@ -143,7 +146,7 @@ TEST(LocateSymbol, CompleteSymbolHint) {
{
// stdlib symbols are always complete.
LocateExample Test("namespace std { struct vector; }");
- EXPECT_THAT(locateSymbol(Test.findDecl("vector")),
+ EXPECT_THAT(locateSymbol(Test.findDecl("vector"), Test.langOpts()),
ElementsAre(HintedSymbol(
*tooling::stdlib::Symbol::named("std::", "vector"),
Hints::CompleteSymbol)));
@@ -151,7 +154,7 @@ TEST(LocateSymbol, CompleteSymbolHint) {
{
// macros are always complete.
LocateExample Test("#define ^FOO");
- EXPECT_THAT(locateSymbol(Test.findMacro("FOO")),
+ EXPECT_THAT(locateSymbol(Test.findMacro("FOO"), Test.langOpts()),
ElementsAre(HintedSymbol(Test.points().front(),
Hints::CompleteSymbol)));
}
@@ -165,7 +168,7 @@ TEST(LocateSymbol, CompleteSymbolHint) {
for (auto &Case : Cases) {
SCOPED_TRACE(Case);
LocateExample Test(Case);
- EXPECT_THAT(locateSymbol(Test.findDecl("foo")),
+ EXPECT_THAT(locateSymbol(Test.findDecl("foo"), Test.langOpts()),
ElementsAre(HintedSymbol(Test.points().front(), Hints::None),
HintedSymbol(Test.points().back(),
Hints::CompleteSymbol)));
@@ -181,7 +184,7 @@ TEST(LocateSymbol, CompleteSymbolHint) {
for (auto &Case : Cases) {
SCOPED_TRACE(Case);
LocateExample Test(Case);
- EXPECT_THAT(locateSymbol(Test.findDecl("foo")),
+ EXPECT_THAT(locateSymbol(Test.findDecl("foo"), Test.langOpts()),
Each(Field(&Hinted<SymbolLocation>::Hint,
Eq(Hints::CompleteSymbol))));
}
|
hokein
approved these changes
Jan 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #113926.
Fixes #63976.