llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangd Author: kadir çetinkaya (kadircet) <details> <summary>Changes</summary> Fixes https://github.com/llvm/llvm-project/issues/113926. Fixes https://github.com/llvm/llvm-project/issues/63976. --- Full diff: https://github.com/llvm/llvm-project/pull/123634.diff 11 Files Affected: - (modified) clang-tools-extra/clangd/Hover.cpp (+3-2) - (modified) clang-tools-extra/clangd/index/SymbolCollector.cpp (+1-1) - (modified) clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h (+1-1) - (modified) clang-tools-extra/include-cleaner/lib/Analysis.cpp (+2-2) - (modified) clang-tools-extra/include-cleaner/lib/AnalysisInternal.h (+5-2) - (modified) clang-tools-extra/include-cleaner/lib/FindHeaders.cpp (+4-2) - (modified) clang-tools-extra/include-cleaner/lib/HTMLReport.cpp (+10-8) - (modified) clang-tools-extra/include-cleaner/lib/LocateSymbol.cpp (+8-4) - (modified) clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp (+3-4) - (modified) clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp (+29-6) - (modified) clang-tools-extra/include-cleaner/unittests/LocateSymbolTest.cpp (+11-8) ``````````diff 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)))); } `````````` </details> https://github.com/llvm/llvm-project/pull/123634 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits