[clang] 43fcfdb - [IncludeCleaner][clangd] Mark umbrella headers as users of private
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 23 05:09:05 PDT 2023
Author: Kadir Cetinkaya
Date: 2023-03-23T13:08:07+01:00
New Revision: 43fcfdb1d6a63129ffbb7d77174ccb56863d0b30
URL: https://github.com/llvm/llvm-project/commit/43fcfdb1d6a63129ffbb7d77174ccb56863d0b30
DIFF: https://github.com/llvm/llvm-project/commit/43fcfdb1d6a63129ffbb7d77174ccb56863d0b30.diff
LOG: [IncludeCleaner][clangd] Mark umbrella headers as users of private
Private headers inside umbrella files shouldn't be marked as unused.
Differential Revision: https://reviews.llvm.org/D146406
Added:
Modified:
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
clang-tools-extra/include-cleaner/lib/Analysis.cpp
clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
clang/include/clang/Testing/TestAST.h
clang/lib/Testing/TestAST.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp
index 98135529f259..ee470bd8b963 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -93,8 +93,6 @@ bool isFilteredByConfig(const Config &Cfg, llvm::StringRef HeaderPath) {
static bool mayConsiderUnused(const Inclusion &Inc, ParsedAST &AST,
const Config &Cfg,
const include_cleaner::PragmaIncludes *PI) {
- if (PI && PI->shouldKeep(Inc.HashLine + 1))
- return false;
// FIXME(kirillbobyrev): We currently do not support the umbrella headers.
// System headers are likely to be standard library headers.
// Until we have good support for umbrella headers, don't warn about them.
@@ -108,6 +106,20 @@ static bool mayConsiderUnused(const Inclusion &Inc, ParsedAST &AST,
auto FE = AST.getSourceManager().getFileManager().getFileRef(
AST.getIncludeStructure().getRealPath(HID));
assert(FE);
+ if (PI) {
+ if (PI->shouldKeep(Inc.HashLine + 1))
+ return false;
+ // Check if main file is the public interface for a private header. If so we
+ // shouldn't diagnose it as unused.
+ if(auto PHeader = PI->getPublic(*FE); !PHeader.empty()) {
+ PHeader = PHeader.trim("<>\"");
+ // Since most private -> public mappings happen in a verbatim way, we
+ // check textually here. This might go wrong in presence of symlinks or
+ // header mappings. But that's not
diff erent than rest of the places.
+ if(AST.tuPath().endswith(PHeader))
+ return false;
+ }
+ }
// Headers without include guards have side effects and are not
// self-contained, skip them.
if (!AST.getPreprocessor().getHeaderSearchInfo().isFileMultipleIncludeGuarded(
diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
index 409e3cee791c..69b4e07439c3 100644
--- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -30,6 +30,7 @@
#include "gtest/gtest.h"
#include <cstddef>
#include <string>
+#include <utility>
#include <vector>
namespace clang {
@@ -328,6 +329,26 @@ TEST(IncludeCleaner, NoDiagsForObjC) {
ParsedAST AST = TU.build();
EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
}
+
+TEST(IncludeCleaner, UmbrellaUsesPrivate) {
+ TestTU TU;
+ TU.Code = R"cpp(
+ #include "private.h"
+ )cpp";
+ TU.AdditionalFiles["private.h"] = guard(R"cpp(
+ // IWYU pragma: private, include "public.h"
+ void foo() {}
+ )cpp");
+ TU.Filename = "public.h";
+ Config Cfg;
+ Cfg.Diagnostics.UnusedIncludes = Config::IncludesPolicy::Strict;
+ WithContextValue Ctx(Config::Key, std::move(Cfg));
+ ParsedAST AST = TU.build();
+ EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
+ IncludeCleanerFindings Findings = computeIncludeCleanerFindings(AST);
+ EXPECT_THAT(Findings.UnusedIncludes, IsEmpty());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp b/clang-tools-extra/include-cleaner/lib/Analysis.cpp
index 6237bdb46bab..fb0879b7aab6 100644
--- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp
+++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp
@@ -90,9 +90,25 @@ AnalysisResults analyze(llvm::ArrayRef<Decl *> ASTRoots,
});
AnalysisResults Results;
- for (const Include &I : Inc.all())
- if (!Used.contains(&I) && PI && !PI->shouldKeep(I.Line))
- Results.Unused.push_back(&I);
+ for (const Include &I : Inc.all()) {
+ if (Used.contains(&I))
+ continue;
+ if (PI) {
+ if (PI->shouldKeep(I.Line))
+ continue;
+ // Check if main file is the public interface for a private header. If so
+ // we shouldn't diagnose it as unused.
+ if (auto PHeader = PI->getPublic(I.Resolved); !PHeader.empty()) {
+ PHeader = PHeader.trim("<>\"");
+ // Since most private -> public mappings happen in a verbatim way, we
+ // check textually here. This might go wrong in presence of symlinks or
+ // header mappings. But that's not
diff erent than rest of the places.
+ if (MainFile->tryGetRealPathName().endswith(PHeader))
+ continue;
+ }
+ }
+ Results.Unused.push_back(&I);
+ }
for (llvm::StringRef S : Missing.keys())
Results.Missing.push_back(S.str());
llvm::sort(Results.Missing);
diff --git a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
index c34c6c0a29a8..a2084d4f3790 100644
--- a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
@@ -24,6 +24,7 @@
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include <cstddef>
+#include <vector>
namespace clang::include_cleaner {
namespace {
@@ -212,17 +213,34 @@ int x = a + c;
return std::make_unique<Hook>(PP, PI);
};
- TestAST AST(Inputs);
- 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());
+ {
+ TestAST AST(Inputs);
+ 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());
+
+ const Include *B = PP.Includes.atLine(3);
+ ASSERT_EQ(B->Spelled, "b.h");
+ EXPECT_THAT(Results.Missing, ElementsAre("\"c.h\""));
+ EXPECT_THAT(Results.Unused, ElementsAre(B));
+ }
- const Include *B = PP.Includes.atLine(3);
- ASSERT_EQ(B->Spelled, "b.h");
- EXPECT_THAT(Results.Missing, ElementsAre("\"c.h\""));
- EXPECT_THAT(Results.Unused, ElementsAre(B));
+ // Check that umbrella header uses private include.
+ {
+ Inputs.Code = R"cpp(#include "private.h")cpp";
+ Inputs.ExtraFiles["private.h"] =
+ guard("// IWYU pragma: private, include \"public.h\"");
+ Inputs.FileName = "public.h";
+ PP.Includes = {};
+ PI = {};
+ TestAST AST(Inputs);
+ EXPECT_FALSE(PP.Includes.all().empty());
+ auto Results = analyze({}, {}, PP.Includes, &PI, AST.sourceManager(),
+ AST.preprocessor().getHeaderSearchInfo());
+ EXPECT_THAT(Results.Unused, testing::IsEmpty());
+ }
}
TEST(FixIncludes, Basic) {
diff --git a/clang/include/clang/Testing/TestAST.h b/clang/include/clang/Testing/TestAST.h
index 7ba2ca882b91..845e31f65438 100644
--- a/clang/include/clang/Testing/TestAST.h
+++ b/clang/include/clang/Testing/TestAST.h
@@ -49,6 +49,9 @@ struct TestInputs {
/// Keys are plain filenames ("foo.h"), values are file content.
llvm::StringMap<std::string> ExtraFiles = {};
+ /// Filename to use for translation unit. A default will be used when empty.
+ std::string FileName;
+
/// By default, error diagnostics during parsing are reported as gtest errors.
/// To suppress this, set ErrorOK or include "error-ok" in a comment in Code.
/// In either case, all diagnostics appear in TestAST::diagnostics().
diff --git a/clang/lib/Testing/TestAST.cpp b/clang/lib/Testing/TestAST.cpp
index 8c79fcd7d636..3a50c2d9b5d0 100644
--- a/clang/lib/Testing/TestAST.cpp
+++ b/clang/lib/Testing/TestAST.cpp
@@ -16,6 +16,7 @@
#include "llvm/Support/VirtualFileSystem.h"
#include "gtest/gtest.h"
+#include <string>
namespace clang {
namespace {
@@ -91,7 +92,9 @@ TestAST::TestAST(const TestInputs &In) {
Argv.push_back(S.c_str());
for (const auto &S : In.ExtraArgs)
Argv.push_back(S.c_str());
- std::string Filename = getFilenameForTesting(In.Language).str();
+ std::string Filename = In.FileName;
+ if (Filename.empty())
+ Filename = getFilenameForTesting(In.Language).str();
Argv.push_back(Filename.c_str());
Clang->setInvocation(std::make_unique<CompilerInvocation>());
if (!CompilerInvocation::CreateFromArgs(Clang->getInvocation(), Argv,
More information about the cfe-commits
mailing list