hokein updated this revision to Diff 478228.
hokein marked 3 inline comments as done.
hokein added a comment.
address comments:
- polish and simplify the tests
- inline the spelling-loc-check logic
- add a comment for the walkUsed
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138779/new/
https://reviews.llvm.org/D138779
Files:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h
clang-tools-extra/include-cleaner/lib/Analysis.cpp
clang-tools-extra/include-cleaner/lib/HTMLReport.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
@@ -18,13 +18,16 @@
#include "clang/Tooling/Inclusions/StandardLibrary.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/ScopedPrinter.h"
#include "llvm/Testing/Support/Annotations.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include <cstddef>
+#include <optional>
namespace clang::include_cleaner {
namespace {
+using testing::AllOf;
using testing::Pair;
using testing::UnorderedElementsAre;
@@ -134,6 +137,127 @@
UnorderedElementsAre(Pair(Main.point(), UnorderedElementsAre(HdrFile))));
}
+MATCHER_P3(expandedAt, FileID, Offset, SM, "") {
+ auto [ExpanedFileID, ExpandedOffset] = SM->getDecomposedExpansionLoc(arg);
+ return ExpanedFileID == FileID && ExpandedOffset == Offset;
+}
+MATCHER_P3(spelledAt, FileID, Offset, SM, "") {
+ auto [SpelledFileID, SpelledOffset] = SM->getDecomposedSpellingLoc(arg);
+ return SpelledFileID == FileID && SpelledOffset == Offset;
+}
+TEST(WalkUsed, FilterRefsNotSpelledInMainFile) {
+ // Each test is expected to have a single expected ref of `target` symbol
+ // (or have none).
+ // The location in the reported ref is a macro location. $expand points to
+ // the macro location, and $spell points to the spelled location.
+ struct {
+ llvm::StringRef Header;
+ llvm::StringRef Main;
+ } TestCases[] = {
+ // Tests for decl references.
+ {
+ /*Header=*/"",
+ R"cpp(
+ int target();
+ #define CALL_FUNC $spell^target()
+
+ int b = $expand^CALL_FUNC;
+ )cpp",
+ },
+ {/*Header=*/R"cpp(
+ int target();
+ #define CALL_FUNC target()
+ )cpp",
+ // No ref of `target` being reported, as it is not spelled in main file.
+ "int a = CALL_FUNC;"},
+ {
+ /*Header=*/"",
+ R"cpp(
+ int target;
+ #define PLUS_ONE(X) X + 1
+ int a = $expand^PLUS_ONE($spell^target);
+ )cpp",
+ },
+ {
+ /*Header*/ R"cpp(
+ int target;
+ #define PLUS_ONE(X) X + 1
+ )cpp",
+ R"cpp(
+ int a = $expand^PLUS_ONE($spell^target);
+ )cpp",
+ },
+ {
+ /*Header=*/"#define PLUS_ONE(X) X + 1",
+ R"cpp(
+ int target;
+ int a = $expand^PLUS_ONE($spell^target);
+ )cpp",
+ },
+ // Tests for macro references,
+ {/*Header=*/"",
+ R"cpp(
+ #define target 1
+ #define USE_target $spell^target
+ int b = $expand^USE_target;
+ )cpp"},
+ {/*Header=*/R"cpp(
+ #define target 1
+ #define USE_target target
+ )cpp",
+ // No ref of `target` being reported, it is not spelled in main file.
+ R"cpp(
+ int a = USE_target;
+ )cpp"},
+ };
+
+ for (const auto &T : TestCases) {
+ llvm::Annotations Main(T.Main);
+ TestInputs Inputs(Main.code());
+ Inputs.ExtraFiles["header.h"] = guard(T.Header);
+ RecordedPP Recorded;
+ Inputs.MakeAction = [&]() {
+ struct RecordAction : public SyntaxOnlyAction {
+ RecordedPP &Out;
+ RecordAction(RecordedPP &Out) : Out(Out) {}
+ bool BeginSourceFileAction(clang::CompilerInstance &CI) override {
+ auto &PP = CI.getPreprocessor();
+ PP.addPPCallbacks(Out.record(PP));
+ return true;
+ }
+ };
+ return std::make_unique<RecordAction>(Recorded);
+ };
+ Inputs.ExtraArgs.push_back("-include");
+ Inputs.ExtraArgs.push_back("header.h");
+ TestAST AST(Inputs);
+ llvm::SmallVector<Decl *> TopLevelDecls;
+ for (Decl *D : AST.context().getTranslationUnitDecl()->decls())
+ TopLevelDecls.emplace_back(D);
+ auto &SM = AST.sourceManager();
+
+ std::optional<SourceLocation> RefLoc;
+ walkUsed(TopLevelDecls, Recorded.MacroReferences,
+ /*PragmaIncludes=*/nullptr, SM,
+ [&](const SymbolReference &Ref, llvm::ArrayRef<Header>) {
+ if (!Ref.RefLocation.isMacroID())
+ return;
+ if (llvm::to_string(Ref.Target) == "target") {
+ ASSERT_FALSE(RefLoc)
+ << "Expected only one 'target' ref loc per testcase";
+ RefLoc = Ref.RefLocation;
+ }
+ });
+ FileID MainFID = SM.getMainFileID();
+ if (RefLoc) {
+ EXPECT_THAT(*RefLoc, AllOf(expandedAt(MainFID, Main.point("expand"), &SM),
+ spelledAt(MainFID, Main.point("spell"), &SM)))
+ << T.Main;
+ } else {
+ EXPECT_THAT(Main.points(), testing::IsEmpty());
+ }
+ }
+}
} // namespace
} // namespace clang::include_cleaner
Index: clang-tools-extra/include-cleaner/lib/HTMLReport.cpp
===================================================================
--- clang-tools-extra/include-cleaner/lib/HTMLReport.cpp
+++ clang-tools-extra/include-cleaner/lib/HTMLReport.cpp
@@ -527,12 +527,18 @@
HeaderSearch &HS, PragmaIncludes *PI,
llvm::raw_ostream &OS) {
Reporter R(OS, Ctx, HS, Includes, PI, File);
+ const auto& SM = Ctx.getSourceManager();
for (Decl *Root : Roots)
walkAST(*Root, [&](SourceLocation Loc, const NamedDecl &D, RefType T) {
+ if(!SM.isWrittenInMainFile(SM.getSpellingLoc(Loc)))
+ return;
R.addRef(SymbolReference{Loc, D, T});
});
- for (const SymbolReference &Ref : MacroRefs)
+ for (const SymbolReference &Ref : MacroRefs) {
+ if (!SM.isWrittenInMainFile(SM.getSpellingLoc(Ref.RefLocation)))
+ continue;
R.addRef(Ref);
+ }
R.write();
}
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
@@ -24,8 +24,9 @@
// This is duplicated in writeHTMLReport, changes should be mirrored there.
tooling::stdlib::Recognizer Recognizer;
for (auto *Root : ASTRoots) {
- auto &SM = Root->getASTContext().getSourceManager();
walkAST(*Root, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) {
+ if (!SM.isWrittenInMainFile(SM.getSpellingLoc(Loc)))
+ return;
SymbolReference SymRef{Loc, ND, RT};
if (auto SS = Recognizer(&ND)) {
// FIXME: Also report forward decls from main-file, so that the caller
@@ -38,6 +39,8 @@
}
for (const SymbolReference &MacroRef : MacroRefs) {
assert(MacroRef.Target.kind() == Symbol::Macro);
+ if (!SM.isWrittenInMainFile(SM.getSpellingLoc(MacroRef.RefLocation)))
+ continue;
CB(MacroRef, findHeaders(MacroRef.Target.macro().Definition, SM, PI));
}
}
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
@@ -33,6 +33,7 @@
llvm::ArrayRef<Header> Providers)>;
/// Find and report all references to symbols in a region of code.
+/// It will filter out references that are not spelled in the main file.
///
/// The AST traversal is rooted at ASTRoots - typically top-level declarations
/// of a single source file.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
