[clang-tools-extra] 90ecadd - [include-cleaner] Filter references to identity macros
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 18 05:42:54 PDT 2023
Author: Kadir Cetinkaya
Date: 2023-08-18T14:40:03+02:00
New Revision: 90ecadde62f30275c35fdf7928e3477a41691d21
URL: https://github.com/llvm/llvm-project/commit/90ecadde62f30275c35fdf7928e3477a41691d21
DIFF: https://github.com/llvm/llvm-project/commit/90ecadde62f30275c35fdf7928e3477a41691d21.diff
LOG: [include-cleaner] Filter references to identity macros
Despite being true positives, these results just confuse users. So
filter them out.
Differential Revision: https://reviews.llvm.org/D157905
Added:
Modified:
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
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp b/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
index f47609b19badfc..2658c4b38702ca 100644
--- a/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
@@ -87,7 +87,7 @@ void IncludeCleanerCheck::registerPPCallbacks(const SourceManager &SM,
Preprocessor *PP,
Preprocessor *ModuleExpanderPP) {
PP->addPPCallbacks(RecordedPreprocessor.record(*PP));
- HS = &PP->getHeaderSearchInfo();
+ this->PP = PP;
RecordedPI.record(*PP);
}
@@ -121,7 +121,7 @@ void IncludeCleanerCheck::check(const MatchFinder::MatchResult &Result) {
// 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 @@ void IncludeCleanerCheck::check(const MatchFinder::MatchResult &Result) {
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
diff --git a/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h b/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h
index 9641c7fd9dfcf6..b46e409bd6f6a0 100644
--- a/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h
+++ b/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 @@ class IncludeCleanerCheck : public ClangTidyCheck {
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;
diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp
index aaa160bb048ef5..0ec85fc24df151 100644
--- a/clang-tools-extra/clangd/Hover.cpp
+++ b/clang-tools-extra/clangd/Hover.cpp
@@ -1245,12 +1245,11 @@ std::string getSymbolName(include_cleaner::Symbol Sym) {
}
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 ||
diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp
index 07c6937ac10d5b..1fcb5c7228fb63 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -394,7 +394,7 @@ IncludeCleanerFindings computeIncludeCleanerFindings(ParsedAST &AST) {
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;
diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp
index bcbd214a725cea..9beb3f1417b9aa 100644
--- a/clang-tools-extra/clangd/XRefs.cpp
+++ b/clang-tools-extra/clangd/XRefs.cpp
@@ -1339,7 +1339,7 @@ maybeFindIncludeReferences(ParsedAST &AST, Position Pos,
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 ||
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 77be4f1dad5212..cd2111cf72abf2 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
@@ -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 @@ using UsedSymbolCB = llvm::function_ref<void(const SymbolReference &SymRef,
/// 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 @@ struct AnalysisResults {
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.
diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp b/clang-tools-extra/include-cleaner/lib/Analysis.cpp
index d1c7eda0266576..630be33e791690 100644
--- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp
+++ b/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 @@ void walkUsed(llvm::ArrayRef<Decl *> ASTRoots,
}
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 @@ void walkUsed(llvm::ArrayRef<Decl *> ASTRoots,
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 @@ analyze(llvm::ArrayRef<Decl *> ASTRoots,
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;
diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
index 193fdaeeb5e4c8..30aaee29b9a397 100644
--- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
+++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
@@ -153,14 +153,14 @@ class Action : public clang::ASTFrontendAction {
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)
diff --git a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
index d5f79f54c81db3..daa5f91463ec27 100644
--- a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
@@ -66,8 +66,9 @@ class WalkUsedTest : public testing::Test {
}
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 @@ class WalkUsedTest : public testing::Test {
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 @@ TEST_F(WalkUsedTest, Basic) {
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 @@ TEST_F(WalkUsedTest, MultipleProviders) {
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 @@ TEST_F(WalkUsedTest, MacroRefs) {
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 @@ int x = a + c;
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 @@ TEST_F(AnalyzeTest, PrivateUsedInPublic) {
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 @@ TEST_F(AnalyzeTest, NoCrashWhenUnresolved) {
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 @@ TEST(WalkUsed, FilterRefsNotSpelledInMainFile) {
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 @@ TEST_F(WalkUsedTest, TemplateDecls) {
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 @@ TEST_F(WalkUsedTest, TemplateDecls) {
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
More information about the cfe-commits
mailing list