This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG43fcfdb1d6a6: [IncludeCleaner][clangd] Mark umbrella headers
as users of private (authored by kadircet).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146406/new/
https://reviews.llvm.org/D146406
Files:
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
Index: clang/lib/Testing/TestAST.cpp
===================================================================
--- clang/lib/Testing/TestAST.cpp
+++ 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 @@
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,
Index: clang/include/clang/Testing/TestAST.h
===================================================================
--- clang/include/clang/Testing/TestAST.h
+++ clang/include/clang/Testing/TestAST.h
@@ -49,6 +49,9 @@
/// 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().
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
@@ -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 @@
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) {
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
@@ -90,9 +90,25 @@
});
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 different 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);
Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ 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 @@
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
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -93,8 +93,6 @@
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 @@
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 different 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(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
