This revision was automatically updated to reflect the committed changes.
kadircet marked an inline comment as done.
Closed by commit rG90ecadde62f3: [include-cleaner] Filter references to
identity macros (authored by kadircet).
Changed prior to commit:
https://reviews.llvm.org/D157905?vs=550790&id=551477#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157905/new/
https://reviews.llvm.org/D157905
Files:
clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h
clang-tools-extra/clangd/Hover.cpp
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h
clang-tools-extra/include-cleaner/lib/Analysis.cpp
clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
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
@@ -66,8 +66,9 @@
}
std::multimap<size_t, std::vector<Header>>
- offsetToProviders(TestAST &AST, SourceManager &SM,
+ offsetToProviders(TestAST &AST,
llvm::ArrayRef<SymbolReference> MacroRefs = {}) {
+ const auto &SM = AST.sourceManager();
llvm::SmallVector<Decl *> TopLevelDecls;
for (Decl *D : AST.context().getTranslationUnitDecl()->decls()) {
if (!SM.isWrittenInMainFile(SM.getExpansionLoc(D->getLocation())))
@@ -75,7 +76,7 @@
TopLevelDecls.emplace_back(D);
}
std::multimap<size_t, std::vector<Header>> OffsetToProviders;
- walkUsed(TopLevelDecls, MacroRefs, &PI, SM,
+ walkUsed(TopLevelDecls, MacroRefs, &PI, AST.preprocessor(),
[&](const SymbolReference &Ref, llvm::ArrayRef<Header> Providers) {
auto [FID, Offset] = SM.getDecomposedLoc(Ref.RefLocation);
if (FID != SM.getMainFileID())
@@ -118,7 +119,7 @@
auto VectorSTL = Header(*tooling::stdlib::Header::named("<vector>"));
auto UtilitySTL = Header(*tooling::stdlib::Header::named("<utility>"));
EXPECT_THAT(
- offsetToProviders(AST, SM),
+ offsetToProviders(AST),
UnorderedElementsAre(
Pair(Code.point("bar"), UnorderedElementsAre(MainFile)),
Pair(Code.point("private"),
@@ -155,7 +156,7 @@
auto HeaderFile2 = Header(AST.fileManager().getFile("header2.h").get());
auto MainFile = Header(SM.getFileEntryForID(SM.getMainFileID()));
EXPECT_THAT(
- offsetToProviders(AST, SM),
+ offsetToProviders(AST),
Contains(Pair(Code.point("foo"),
UnorderedElementsAre(HeaderFile1, HeaderFile2, MainFile))));
}
@@ -171,19 +172,19 @@
Inputs.ExtraFiles["hdr.h"] = Hdr.code();
TestAST AST(Inputs);
auto &SM = AST.sourceManager();
+ auto &PP = AST.preprocessor();
const auto *HdrFile = SM.getFileManager().getFile("hdr.h").get();
auto MainFile = Header(SM.getFileEntryForID(SM.getMainFileID()));
auto HdrID = SM.translateFile(HdrFile);
- IdentifierTable Idents;
- Symbol Answer1 =
- Macro{&Idents.get("ANSWER"), SM.getComposedLoc(HdrID, Hdr.point())};
- Symbol Answer2 =
- Macro{&Idents.get("ANSWER"), SM.getComposedLoc(HdrID, Hdr.point())};
+ Symbol Answer1 = Macro{PP.getIdentifierInfo("ANSWER"),
+ SM.getComposedLoc(HdrID, Hdr.point())};
+ Symbol Answer2 = Macro{PP.getIdentifierInfo("ANSWER"),
+ SM.getComposedLoc(HdrID, Hdr.point())};
EXPECT_THAT(
offsetToProviders(
- AST, SM,
+ AST,
{SymbolReference{
Answer1, SM.getComposedLoc(SM.getMainFileID(), Code.point("1")),
RefType::Explicit},
@@ -240,8 +241,7 @@
auto Decls = AST.context().getTranslationUnitDecl()->decls();
auto Results =
analyze(std::vector<Decl *>{Decls.begin(), Decls.end()},
- PP.MacroReferences, PP.Includes, &PI, AST.sourceManager(),
- AST.preprocessor().getHeaderSearchInfo());
+ PP.MacroReferences, PP.Includes, &PI, AST.preprocessor());
const Include *B = PP.Includes.atLine(3);
ASSERT_EQ(B->Spelled, "b.h");
@@ -257,8 +257,7 @@
Inputs.FileName = "public.h";
TestAST AST(Inputs);
EXPECT_FALSE(PP.Includes.all().empty());
- auto Results = analyze({}, {}, PP.Includes, &PI, AST.sourceManager(),
- AST.preprocessor().getHeaderSearchInfo());
+ auto Results = analyze({}, {}, PP.Includes, &PI, AST.preprocessor());
EXPECT_THAT(Results.Unused, testing::IsEmpty());
}
@@ -268,8 +267,7 @@
Inputs.ErrorOK = true;
TestAST AST(Inputs);
EXPECT_FALSE(PP.Includes.all().empty());
- auto Results = analyze({}, {}, PP.Includes, &PI, AST.sourceManager(),
- AST.preprocessor().getHeaderSearchInfo());
+ auto Results = analyze({}, {}, PP.Includes, &PI, AST.preprocessor());
EXPECT_THAT(Results.Unused, testing::IsEmpty());
}
@@ -409,7 +407,7 @@
SourceLocation RefLoc;
walkUsed(TopLevelDecls, Recorded.MacroReferences,
- /*PragmaIncludes=*/nullptr, SM,
+ /*PragmaIncludes=*/nullptr, AST.preprocessor(),
[&](const SymbolReference &Ref, llvm::ArrayRef<Header>) {
if (!Ref.RefLocation.isMacroID())
return;
@@ -472,7 +470,7 @@
const auto *Partial = SM.getFileManager().getFile("partial.h").get();
EXPECT_THAT(
- offsetToProviders(AST, SM),
+ offsetToProviders(AST),
AllOf(Contains(
Pair(Code.point("exp_spec"), UnorderedElementsAre(Fwd, Def))),
Contains(Pair(Code.point("exp"), UnorderedElementsAre(Fwd, Def))),
@@ -483,5 +481,29 @@
Pair(Code.point("partial"), UnorderedElementsAre(Partial)))));
}
+TEST_F(WalkUsedTest, IgnoresIdentityMacros) {
+ llvm::Annotations Code(R"cpp(
+ #include "header.h"
+ void $bar^bar() {
+ $stdin^stdin();
+ }
+ )cpp");
+ Inputs.Code = Code.code();
+ Inputs.ExtraFiles["header.h"] = guard(R"cpp(
+ #include "inner.h"
+ void stdin();
+ )cpp");
+ Inputs.ExtraFiles["inner.h"] = guard(R"cpp(
+ #define stdin stdin
+ )cpp");
+
+ TestAST AST(Inputs);
+ auto &SM = AST.sourceManager();
+ auto MainFile = Header(SM.getFileEntryForID(SM.getMainFileID()));
+ EXPECT_THAT(offsetToProviders(AST),
+ UnorderedElementsAre(
+ // FIXME: we should have a reference from stdin to header.h
+ Pair(Code.point("bar"), UnorderedElementsAre(MainFile))));
+}
} // namespace
} // namespace clang::include_cleaner
Index: clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
+++ clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
@@ -153,14 +153,14 @@
if (!HTMLReportPath.empty())
writeHTML();
- auto &HS = getCompilerInstance().getPreprocessor().getHeaderSearchInfo();
llvm::StringRef Path =
SM.getFileEntryForID(SM.getMainFileID())->tryGetRealPathName();
assert(!Path.empty() && "Main file path not known?");
llvm::StringRef Code = SM.getBufferData(SM.getMainFileID());
- auto Results = analyze(AST.Roots, PP.MacroReferences, PP.Includes, &PI, SM,
- HS, HeaderFilter);
+ auto Results =
+ analyze(AST.Roots, PP.MacroReferences, PP.Includes, &PI,
+ getCompilerInstance().getPreprocessor(), HeaderFilter);
if (!Insert)
Results.Missing.clear();
if (!Remove)
Index: clang-tools-extra/include-cleaner/lib/Analysis.cpp
===================================================================
--- clang-tools-extra/include-cleaner/lib/Analysis.cpp
+++ clang-tools-extra/include-cleaner/lib/Analysis.cpp
@@ -17,6 +17,7 @@
#include "clang/Basic/SourceManager.h"
#include "clang/Format/Format.h"
#include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/Preprocessor.h"
#include "clang/Tooling/Core/Replacement.h"
#include "clang/Tooling/Inclusions/StandardLibrary.h"
#include "llvm/ADT/ArrayRef.h"
@@ -28,14 +29,30 @@
#include "llvm/ADT/StringSet.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorHandling.h"
+#include <cassert>
+#include <climits>
#include <string>
namespace clang::include_cleaner {
+namespace {
+bool shouldIgnoreMacroReference(const Preprocessor &PP, const Macro &M) {
+ auto *MI = PP.getMacroInfo(M.Name);
+ // Macros that expand to themselves are confusing from user's point of view.
+ // They usually aspect the usage to be attributed to the underlying decl and
+ // not the macro definition. So ignore such macros (e.g. std{in,out,err} are
+ // implementation defined macros, that just resolve to themselves in
+ // practice).
+ return MI && MI->getNumTokens() == 1 && MI->isObjectLike() &&
+ MI->getReplacementToken(0).getIdentifierInfo() == M.Name;
+}
+} // namespace
+
void walkUsed(llvm::ArrayRef<Decl *> ASTRoots,
llvm::ArrayRef<SymbolReference> MacroRefs,
- const PragmaIncludes *PI, const SourceManager &SM,
+ const PragmaIncludes *PI, const Preprocessor &PP,
UsedSymbolCB CB) {
+ const auto &SM = PP.getSourceManager();
// This is duplicated in writeHTMLReport, changes should be mirrored there.
tooling::stdlib::Recognizer Recognizer;
for (auto *Root : ASTRoots) {
@@ -51,7 +68,8 @@
}
for (const SymbolReference &MacroRef : MacroRefs) {
assert(MacroRef.Target.kind() == Symbol::Macro);
- if (!SM.isWrittenInMainFile(SM.getSpellingLoc(MacroRef.RefLocation)))
+ if (!SM.isWrittenInMainFile(SM.getSpellingLoc(MacroRef.RefLocation)) ||
+ shouldIgnoreMacroReference(PP, MacroRef.Target.macro()))
continue;
CB(MacroRef, headersForSymbol(MacroRef.Target, SM, PI));
}
@@ -60,15 +78,15 @@
AnalysisResults
analyze(llvm::ArrayRef<Decl *> ASTRoots,
llvm::ArrayRef<SymbolReference> MacroRefs, const Includes &Inc,
- const PragmaIncludes *PI, const SourceManager &SM,
- const HeaderSearch &HS,
+ const PragmaIncludes *PI, const Preprocessor &PP,
llvm::function_ref<bool(llvm::StringRef)> HeaderFilter) {
+ auto &SM = PP.getSourceManager();
const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID());
llvm::DenseSet<const Include *> Used;
llvm::StringSet<> Missing;
if (!HeaderFilter)
HeaderFilter = [](llvm::StringRef) { return false; };
- walkUsed(ASTRoots, MacroRefs, PI, SM,
+ walkUsed(ASTRoots, MacroRefs, PI, PP,
[&](const SymbolReference &Ref, llvm::ArrayRef<Header> Providers) {
bool Satisfied = false;
for (const Header &H : Providers) {
@@ -82,7 +100,8 @@
if (!Satisfied && !Providers.empty() &&
Ref.RT == RefType::Explicit &&
!HeaderFilter(Providers.front().resolvedPath()))
- Missing.insert(spellHeader({Providers.front(), HS, MainFile}));
+ Missing.insert(spellHeader(
+ {Providers.front(), PP.getHeaderSearchInfo(), MainFile}));
});
AnalysisResults Results;
Index: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h
===================================================================
--- clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h
+++ clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h
@@ -15,6 +15,7 @@
#include "clang-include-cleaner/Types.h"
#include "clang/Format/Format.h"
#include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/Preprocessor.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/STLFunctionalExtras.h"
#include "llvm/ADT/SmallVector.h"
@@ -56,7 +57,8 @@
/// the headers for any referenced symbol
void walkUsed(llvm::ArrayRef<Decl *> ASTRoots,
llvm::ArrayRef<SymbolReference> MacroRefs,
- const PragmaIncludes *PI, const SourceManager &, UsedSymbolCB CB);
+ const PragmaIncludes *PI, const Preprocessor &PP,
+ UsedSymbolCB CB);
struct AnalysisResults {
std::vector<const Include *> Unused;
@@ -72,8 +74,7 @@
AnalysisResults
analyze(llvm::ArrayRef<Decl *> ASTRoots,
llvm::ArrayRef<SymbolReference> MacroRefs, const Includes &I,
- const PragmaIncludes *PI, const SourceManager &SM,
- const HeaderSearch &HS,
+ const PragmaIncludes *PI, const Preprocessor &PP,
llvm::function_ref<bool(llvm::StringRef)> HeaderFilter = nullptr);
/// Removes unused includes and inserts missing ones in the main file.
Index: clang-tools-extra/clangd/XRefs.cpp
===================================================================
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -1339,7 +1339,7 @@
auto Converted = convertIncludes(AST);
include_cleaner::walkUsed(
AST.getLocalTopLevelDecls(), collectMacroReferences(AST),
- AST.getPragmaIncludes().get(), SM,
+ AST.getPragmaIncludes().get(), AST.getPreprocessor(),
[&](const include_cleaner::SymbolReference &Ref,
llvm::ArrayRef<include_cleaner::Header> Providers) {
if (Ref.RT != include_cleaner::RefType::Explicit ||
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -394,7 +394,7 @@
trace::Span Tracer("include_cleaner::walkUsed");
include_cleaner::walkUsed(
AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
- AST.getPragmaIncludes().get(), SM,
+ AST.getPragmaIncludes().get(), AST.getPreprocessor(),
[&](const include_cleaner::SymbolReference &Ref,
llvm::ArrayRef<include_cleaner::Header> Providers) {
bool Satisfied = false;
Index: clang-tools-extra/clangd/Hover.cpp
===================================================================
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -1245,12 +1245,11 @@
}
void maybeAddUsedSymbols(ParsedAST &AST, HoverInfo &HI, const Inclusion &Inc) {
- const SourceManager &SM = AST.getSourceManager();
auto Converted = convertIncludes(AST);
llvm::DenseSet<include_cleaner::Symbol> UsedSymbols;
include_cleaner::walkUsed(
AST.getLocalTopLevelDecls(), collectMacroReferences(AST),
- AST.getPragmaIncludes().get(), SM,
+ AST.getPragmaIncludes().get(), AST.getPreprocessor(),
[&](const include_cleaner::SymbolReference &Ref,
llvm::ArrayRef<include_cleaner::Header> Providers) {
if (Ref.RT != include_cleaner::RefType::Explicit ||
Index: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h
+++ clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h
@@ -18,6 +18,7 @@
#include "clang/Basic/LLVM.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/Preprocessor.h"
#include "llvm/Support/Regex.h"
#include <vector>
@@ -42,7 +43,7 @@
private:
include_cleaner::RecordedPP RecordedPreprocessor;
include_cleaner::PragmaIncludes RecordedPI;
- HeaderSearch *HS;
+ const Preprocessor *PP = nullptr;
std::vector<StringRef> IgnoreHeaders;
// Whether emit only one finding per usage of a symbol.
const bool DeduplicateFindings;
Index: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
@@ -87,7 +87,7 @@
Preprocessor *PP,
Preprocessor *ModuleExpanderPP) {
PP->addPPCallbacks(RecordedPreprocessor.record(*PP));
- HS = &PP->getHeaderSearchInfo();
+ this->PP = PP;
RecordedPI.record(*PP);
}
@@ -121,7 +121,7 @@
// FIXME: Find a way to have less code duplication between include-cleaner
// analysis implementation and the below code.
walkUsed(MainFileDecls, RecordedPreprocessor.MacroReferences, &RecordedPI,
- *SM,
+ *PP,
[&](const include_cleaner::SymbolReference &Ref,
llvm::ArrayRef<include_cleaner::Header> Providers) {
// Process each symbol once to reduce noise in the findings.
@@ -200,8 +200,8 @@
tooling::HeaderIncludes HeaderIncludes(getCurrentMainFile(), Code,
FileStyle->IncludeStyle);
for (const auto &Inc : Missing) {
- std::string Spelling =
- include_cleaner::spellHeader({Inc.Missing, *HS, MainFile});
+ std::string Spelling = include_cleaner::spellHeader(
+ {Inc.Missing, PP->getHeaderSearchInfo(), MainFile});
bool Angled = llvm::StringRef{Spelling}.starts_with("<");
// We might suggest insertion of an existing include in edge cases, e.g.,
// include is present in a PP-disabled region, or spelling of the header
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
