VitaNuo updated this revision to Diff 549443.
VitaNuo marked 4 inline comments as done.
VitaNuo added a comment.
Address review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157610/new/
https://reviews.llvm.org/D157610
Files:
clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
clang-tools-extra/include-cleaner/lib/Analysis.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
@@ -273,6 +273,30 @@
EXPECT_THAT(Results.Unused, testing::IsEmpty());
}
+TEST_F(AnalyzeTest, ResourceDirIsIgnored) {
+ Inputs.ExtraArgs.push_back("-resource-dir");
+ Inputs.ExtraArgs.push_back("./resources");
+ Inputs.Code = R"cpp(
+ #include "resources/amintrin.h"
+ #include "resources/imintrin.h"
+ void baz() {
+ bar();
+ }
+ )cpp";
+ Inputs.ExtraFiles["resources/amintrin.h"] = "";
+ Inputs.ExtraFiles["resources/emintrin.h"] = guard(R"cpp(
+ void bar();
+ )cpp");
+ Inputs.ExtraFiles["resources/imintrin.h"] = guard(R"cpp(
+ #include "emintrin.h"
+ )cpp");
+ TestAST AST(Inputs);
+ auto Results = analyze({}, {}, PP.Includes, &PI, AST.sourceManager(),
+ AST.preprocessor().getHeaderSearchInfo());
+ EXPECT_THAT(Results.Unused, testing::IsEmpty());
+ EXPECT_THAT(Results.Missing, testing::IsEmpty());
+}
+
TEST(FixIncludes, Basic) {
llvm::StringRef Code = R"cpp(#include "d.h"
#include "a.h"
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/HeaderSearchOptions.h"
#include "clang/Tooling/Core/Replacement.h"
#include "clang/Tooling/Inclusions/StandardLibrary.h"
#include "llvm/ADT/ArrayRef.h"
@@ -68,12 +69,18 @@
llvm::StringSet<> Missing;
if (!HeaderFilter)
HeaderFilter = [](llvm::StringRef) { return false; };
+ std::string ResourceDir = HS.getHeaderSearchOpts().ResourceDir;
walkUsed(ASTRoots, MacroRefs, PI, SM,
[&](const SymbolReference &Ref, llvm::ArrayRef<Header> Providers) {
bool Satisfied = false;
for (const Header &H : Providers) {
- if (H.kind() == Header::Physical && H.physical() == MainFile)
+ if (H.kind() == Header::Physical &&
+ (H.physical() == MainFile ||
+ H.physical()->getLastRef().getDir().getName() ==
+ ResourceDir)) {
Satisfied = true;
+ continue;
+ }
for (const Include *I : Inc.match(H)) {
Used.insert(I);
Satisfied = true;
@@ -88,7 +95,8 @@
AnalysisResults Results;
for (const Include &I : Inc.all()) {
if (Used.contains(&I) || !I.Resolved ||
- HeaderFilter(I.Resolved->getFileEntry().tryGetRealPathName()))
+ HeaderFilter(I.Resolved->getFileEntry().tryGetRealPathName()) ||
+ I.Resolved->getDir().getName() == ResourceDir)
continue;
if (PI) {
if (PI->shouldKeep(*I.Resolved))
Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -574,6 +574,29 @@
EXPECT_THAT(Findings.UnusedIncludes, IsEmpty());
}
+TEST(IncludeCleaner, ResourceDirIsIgnored) {
+ auto TU = TestTU::withCode(R"cpp(
+ #include <amintrin.h>
+ #include <imintrin.h>
+ void baz() {
+ bar();
+ }
+ )cpp");
+ TU.ExtraArgs.push_back("-resource-dir");
+ TU.ExtraArgs.push_back(testPath("resources"));
+ TU.AdditionalFiles["resources/include/amintrin.h"] = "";
+ TU.AdditionalFiles["resources/include/imintrin.h"] = guard(R"cpp(
+ #include <emintrin.h>
+ )cpp");
+ TU.AdditionalFiles["resources/include/emintrin.h"] = guard(R"cpp(
+ void bar();
+ )cpp");
+ auto AST = TU.build();
+ auto Findings = computeIncludeCleanerFindings(AST);
+ EXPECT_THAT(Findings.UnusedIncludes, IsEmpty());
+ EXPECT_THAT(Findings.MissingIncludes, IsEmpty());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
===================================================================
--- clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
+++ clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
@@ -5,10 +5,11 @@
# RUN: rm -rf %t
# RUN: mkdir -p %t/clangd
# RUN: cp -r %S/Inputs/include-cleaner %t/include
+# RUN: echo '-I%t/include' > %t/compile_flags.txt
# Create a config file enabling include-cleaner features.
# RUN: echo $'Diagnostics:\n UnusedIncludes: Strict\n MissingIncludes: Strict' >> %t/clangd/config.yaml
-# RUN: env XDG_CONFIG_HOME=%t clangd -lit-test -enable-config --resource-dir=%t < %s | FileCheck -strict-whitespace %s
+# RUN: env XDG_CONFIG_HOME=%t clangd -lit-test -enable-config --compile-commands-dir=%t < %s | FileCheck -strict-whitespace %s
{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"workspace":{"workspaceEdit":{"documentChanges":true, "changeAnnotationSupport":{"groupsOnLabel":true}}}},"trace":"off"}}
---
{
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -75,6 +75,11 @@
auto FE = AST.getSourceManager().getFileManager().getFileRef(
AST.getIncludeStructure().getRealPath(HID));
assert(FE);
+ if (FE->getDir() == AST.getPreprocessor()
+ .getHeaderSearchInfo()
+ .getModuleMap()
+ .getBuiltinDir())
+ return false;
if (PI && PI->shouldKeep(*FE))
return false;
// FIXME(kirillbobyrev): We currently do not support the umbrella headers.
@@ -392,6 +397,10 @@
std::vector<MissingIncludeDiagInfo> MissingIncludes;
llvm::DenseSet<IncludeStructure::HeaderID> Used;
trace::Span Tracer("include_cleaner::walkUsed");
+ const DirectoryEntry *ResourceDir = AST.getPreprocessor()
+ .getHeaderSearchInfo()
+ .getModuleMap()
+ .getBuiltinDir();
include_cleaner::walkUsed(
AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
AST.getPragmaIncludes().get(), SM,
@@ -400,7 +409,8 @@
bool Satisfied = false;
for (const auto &H : Providers) {
if (H.kind() == include_cleaner::Header::Physical &&
- (H.physical() == MainFile || H.physical() == PreamblePatch)) {
+ (H.physical() == MainFile || H.physical() == PreamblePatch ||
+ H.physical()->getLastRef().getDir() == ResourceDir)) {
Satisfied = true;
continue;
}
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
@@ -26,6 +26,7 @@
#include "clang/Basic/LangOptions.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Format/Format.h"
+#include "clang/Lex/HeaderSearchOptions.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Tooling/Core/Replacement.h"
#include "clang/Tooling/Inclusions/HeaderIncludes.h"
@@ -118,47 +119,52 @@
MainFileDecls.push_back(D);
}
llvm::DenseSet<include_cleaner::Symbol> SeenSymbols;
+ std::string ResourceDir = HS->getHeaderSearchOpts().ResourceDir;
// FIXME: Find a way to have less code duplication between include-cleaner
// analysis implementation and the below code.
- walkUsed(MainFileDecls, RecordedPreprocessor.MacroReferences, &RecordedPI,
- *SM,
- [&](const include_cleaner::SymbolReference &Ref,
- llvm::ArrayRef<include_cleaner::Header> Providers) {
- // Process each symbol once to reduce noise in the findings.
- // Tidy checks are used in two different workflows:
- // - Ones that show all the findings for a given file. For such
- // workflows there is not much point in showing all the occurences,
- // as one is enough to indicate the issue.
- // - Ones that show only the findings on changed pieces. For such
- // workflows it's useful to show findings on every reference of a
- // symbol as otherwise tools might give incosistent results
- // depending on the parts of the file being edited. But it should
- // still help surface findings for "new violations" (i.e.
- // dependency did not exist in the code at all before).
- if (DeduplicateFindings && !SeenSymbols.insert(Ref.Target).second)
- return;
- bool Satisfied = false;
- for (const include_cleaner::Header &H : Providers) {
- if (H.kind() == include_cleaner::Header::Physical &&
- H.physical() == MainFile)
- Satisfied = true;
-
- for (const include_cleaner::Include *I :
- RecordedPreprocessor.Includes.match(H)) {
- Used.insert(I);
- Satisfied = true;
- }
- }
- if (!Satisfied && !Providers.empty() &&
- Ref.RT == include_cleaner::RefType::Explicit &&
- !shouldIgnore(Providers.front()))
- Missing.push_back({Ref, Providers.front()});
- });
+ walkUsed(
+ MainFileDecls, RecordedPreprocessor.MacroReferences, &RecordedPI, *SM,
+ [&](const include_cleaner::SymbolReference &Ref,
+ llvm::ArrayRef<include_cleaner::Header> Providers) {
+ // Process each symbol once to reduce noise in the findings.
+ // Tidy checks are used in two different workflows:
+ // - Ones that show all the findings for a given file. For such
+ // workflows there is not much point in showing all the occurences,
+ // as one is enough to indicate the issue.
+ // - Ones that show only the findings on changed pieces. For such
+ // workflows it's useful to show findings on every reference of a
+ // symbol as otherwise tools might give incosistent results
+ // depending on the parts of the file being edited. But it should
+ // still help surface findings for "new violations" (i.e.
+ // dependency did not exist in the code at all before).
+ if (DeduplicateFindings && !SeenSymbols.insert(Ref.Target).second)
+ return;
+ bool Satisfied = false;
+ for (const include_cleaner::Header &H : Providers) {
+ if (H.kind() == include_cleaner::Header::Physical &&
+ (H.physical() == MainFile ||
+ H.physical()->getLastRef().getDir().getName() == ResourceDir)) {
+ Satisfied = true;
+ continue;
+ }
+
+ for (const include_cleaner::Include *I :
+ RecordedPreprocessor.Includes.match(H)) {
+ Used.insert(I);
+ Satisfied = true;
+ }
+ }
+ if (!Satisfied && !Providers.empty() &&
+ Ref.RT == include_cleaner::RefType::Explicit &&
+ !shouldIgnore(Providers.front()))
+ Missing.push_back({Ref, Providers.front()});
+ });
std::vector<const include_cleaner::Include *> Unused;
for (const include_cleaner::Include &I :
RecordedPreprocessor.Includes.all()) {
- if (Used.contains(&I) || !I.Resolved)
+ if (Used.contains(&I) || !I.Resolved ||
+ I.Resolved->getDir().getName() == ResourceDir)
continue;
if (RecordedPI.shouldKeep(*I.Resolved))
continue;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
