Skip to content

[include-cleaner] Respect langopts when analyzing macro names #123634

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
Jan 21, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions clang-tools-extra/clangd/Hover.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion clang-tools-extra/clangd/index/SymbolCollector.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions clang-tools-extra/include-cleaner/lib/Analysis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,15 @@ 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) {
assert(MacroRef.Target.kind() == Symbol::Macro);
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));
}
}

Expand Down
7 changes: 5 additions & 2 deletions clang-tools-extra/include-cleaner/lib/AnalysisInternal.h
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand Down Expand Up @@ -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
Expand Down
6 changes: 4 additions & 2 deletions clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -239,16 +240,17 @@ 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.
llvm::SmallVector<Hinted<Header>> Headers;
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
Expand Down
18 changes: 10 additions & 8 deletions clang-tools-extra/include-cleaner/lib/HTMLReport.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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));
Expand All @@ -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)) {}

Expand Down Expand Up @@ -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) {
Expand Down
12 changes: 8 additions & 4 deletions clang-tools-extra/include-cleaner/lib/LocateSymbol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down
7 changes: 3 additions & 4 deletions clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
35 changes: 29 additions & 6 deletions clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"); }
};
Expand Down Expand Up @@ -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"
Expand All @@ -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"
Expand Down
19 changes: 11 additions & 8 deletions clang-tools-extra/include-cleaner/unittests/LocateSymbolTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Expand All @@ -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()));
}
}
Expand All @@ -119,20 +122,20 @@ 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")));
}
}

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()));
}

Expand All @@ -143,15 +146,15 @@ 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)));
}
{
// 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)));
}
Expand All @@ -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)));
Expand All @@ -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))));
}
Expand Down
Loading