[include-cleaner] Attach Header to AnalysisResults for misisng headers #110272
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently callers of analyze can't get detailed information about a missing header, e.g. resolve path. Only way to get at this is to use low level walkUsed funciton, which is way more complicated than just calling analyze. This enables further analysis, e.g. when includes are spelled relative to inner directories, caller can still know their path relative to repository root.
|
@llvm/pr-subscribers-clang-tools-extra Author: kadir çetinkaya (kadircet) ChangesCurrently callers of analyze can't get detailed information about a missing header, e.g. resolve path. Only way to get at this is to use low level walkUsed funciton, which is way more complicated than just calling analyze. This enables further analysis, e.g. when includes are spelled relative to inner directories, caller can still know their path relative to repository root. Full diff: https://github.com/llvm/llvm-project/pull/110272.diff 4 Files Affected:
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 cd2111cf72abf2..46ca3c9d080740 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
@@ -21,6 +21,7 @@
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
#include <string>
+#include <utility>
namespace clang {
class SourceLocation;
@@ -62,7 +63,8 @@ void walkUsed(llvm::ArrayRef<Decl *> ASTRoots,
struct AnalysisResults {
std::vector<const Include *> Unused;
- std::vector<std::string> Missing; // Spellings, like "<vector>"
+ // Spellings, like "<vector>" paired with the Header that generated it.
+ std::vector<std::pair<std::string, Header>> Missing;
};
/// Determine which headers should be inserted or removed from the main file.
diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp b/clang-tools-extra/include-cleaner/lib/Analysis.cpp
index 05e9d14734a95f..16013f53894e8d 100644
--- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp
+++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp
@@ -26,8 +26,8 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/STLFunctionalExtras.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
-#include "llvm/ADT/StringSet.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorHandling.h"
#include <cassert>
@@ -84,7 +84,7 @@ analyze(llvm::ArrayRef<Decl *> ASTRoots,
auto &SM = PP.getSourceManager();
const auto MainFile = *SM.getFileEntryRefForID(SM.getMainFileID());
llvm::DenseSet<const Include *> Used;
- llvm::StringSet<> Missing;
+ llvm::StringMap<Header> Missing;
if (!HeaderFilter)
HeaderFilter = [](llvm::StringRef) { return false; };
OptionalDirectoryEntryRef ResourceDir =
@@ -119,7 +119,7 @@ analyze(llvm::ArrayRef<Decl *> ASTRoots,
Satisfied = true;
}
if (!Satisfied)
- Missing.insert(std::move(Spelling));
+ Missing.try_emplace(std::move(Spelling), Providers.front());
});
AnalysisResults Results;
@@ -144,8 +144,8 @@ analyze(llvm::ArrayRef<Decl *> ASTRoots,
}
Results.Unused.push_back(&I);
}
- for (llvm::StringRef S : Missing.keys())
- Results.Missing.push_back(S.str());
+ for (auto &E : Missing)
+ Results.Missing.emplace_back(E.first().str(), E.second);
llvm::sort(Results.Missing);
return Results;
}
@@ -158,9 +158,9 @@ std::string fixIncludes(const AnalysisResults &Results,
// Encode insertions/deletions in the magic way clang-format understands.
for (const Include *I : Results.Unused)
cantFail(R.add(tooling::Replacement(FileName, UINT_MAX, 1, I->quote())));
- for (llvm::StringRef Spelled : Results.Missing)
- cantFail(R.add(tooling::Replacement(FileName, UINT_MAX, 0,
- ("#include " + Spelled).str())));
+ for (auto &[Spelled, _] : Results.Missing)
+ cantFail(R.add(
+ tooling::Replacement(FileName, UINT_MAX, 0, "#include " + Spelled)));
// "cleanup" actually turns the UINT_MAX replacements into concrete edits.
auto Positioned = cantFail(format::cleanupAroundReplacements(Code, R, Style));
return cantFail(tooling::applyAllReplacements(Code, Positioned));
diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
index afae4365587aea..080099adc9a07a 100644
--- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
+++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
@@ -192,7 +192,7 @@ class Action : public clang::ASTFrontendAction {
case PrintStyle::Changes:
for (const Include *I : Results.Unused)
llvm::outs() << "- " << I->quote() << " @Line:" << I->Line << "\n";
- for (const auto &I : Results.Missing)
+ for (const auto &[I, _] : Results.Missing)
llvm::outs() << "+ " << I << "\n";
break;
case PrintStyle::Final:
diff --git a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
index 43634ee8f2d803..d2d137a0dfb42a 100644
--- a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
@@ -25,6 +25,7 @@
#include "llvm/ADT/IntrusiveRefCntPtr.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/ScopedPrinter.h"
#include "llvm/Support/VirtualFileSystem.h"
@@ -39,6 +40,7 @@
namespace clang::include_cleaner {
namespace {
+using testing::_;
using testing::AllOf;
using testing::Contains;
using testing::ElementsAre;
@@ -262,10 +264,12 @@ int x = a + c;
auto Results =
analyze(std::vector<Decl *>{Decls.begin(), Decls.end()},
PP.MacroReferences, PP.Includes, &PI, AST.preprocessor());
+ auto CHeader = llvm::cantFail(
+ AST.context().getSourceManager().getFileManager().getFileRef("c.h"));
const Include *B = PP.Includes.atLine(3);
ASSERT_EQ(B->Spelled, "b.h");
- EXPECT_THAT(Results.Missing, ElementsAre("\"c.h\""));
+ EXPECT_THAT(Results.Missing, ElementsAre(Pair("\"c.h\"", Header(CHeader))));
EXPECT_THAT(Results.Unused, ElementsAre(B));
}
@@ -370,7 +374,7 @@ TEST_F(AnalyzeTest, SpellingIncludesWithSymlinks) {
auto Results = analyze(DeclsInTU, {}, PP.Includes, &PI, AST.preprocessor());
// Check that we're spelling header using the symlink, and not underlying
// path.
- EXPECT_THAT(Results.Missing, testing::ElementsAre("\"inner.h\""));
+ EXPECT_THAT(Results.Missing, testing::ElementsAre(Pair("\"inner.h\"", _)));
// header.h should be unused.
EXPECT_THAT(Results.Unused, Not(testing::IsEmpty()));
@@ -379,7 +383,7 @@ TEST_F(AnalyzeTest, SpellingIncludesWithSymlinks) {
auto HeaderFilter = [](llvm::StringRef Path) { return Path == "inner.h"; };
Results = analyze(DeclsInTU, {}, PP.Includes, &PI, AST.preprocessor(),
HeaderFilter);
- EXPECT_THAT(Results.Missing, testing::ElementsAre("\"inner.h\""));
+ EXPECT_THAT(Results.Missing, testing::ElementsAre(Pair("\"inner.h\"", _)));
// header.h should be unused.
EXPECT_THAT(Results.Unused, Not(testing::IsEmpty()));
}
@@ -389,7 +393,7 @@ TEST_F(AnalyzeTest, SpellingIncludesWithSymlinks) {
HeaderFilter);
// header.h should be ignored now.
EXPECT_THAT(Results.Unused, Not(testing::IsEmpty()));
- EXPECT_THAT(Results.Missing, testing::ElementsAre("\"inner.h\""));
+ EXPECT_THAT(Results.Missing, testing::ElementsAre(Pair("\"inner.h\"", _)));
}
}
@@ -414,9 +418,9 @@ TEST(FixIncludes, Basic) {
Inc.add(I);
AnalysisResults Results;
- Results.Missing.push_back("\"aa.h\"");
- Results.Missing.push_back("\"ab.h\"");
- Results.Missing.push_back("<e.h>");
+ Results.Missing.emplace_back("\"aa.h\"", Header(""));
+ Results.Missing.emplace_back("\"ab.h\"", Header(""));
+ Results.Missing.emplace_back("<e.h>", Header(""));
Results.Unused.push_back(Inc.atLine(3));
Results.Unused.push_back(Inc.atLine(4));
@@ -429,7 +433,7 @@ R"cpp(#include "d.h"
)cpp");
Results = {};
- Results.Missing.push_back("\"d.h\"");
+ Results.Missing.emplace_back("\"d.h\"", Header(""));
Code = R"cpp(#include "a.h")cpp";
EXPECT_EQ(fixIncludes(Results, "d.cc", Code, format::getLLVMStyle()),
R"cpp(#include "d.h"
|
VitaNuo
approved these changes
Sep 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently callers of analyze can't get detailed information about a missing header, e.g. resolve path. Only way to get at this is to use low level walkUsed funciton, which is way more complicated than just calling analyze.
This enables further analysis, e.g. when includes are spelled relative to inner directories, caller can still know their path relative to repository root.