Author: Kadir Cetinkaya Date: 2023-07-05T15:37:17+02:00 New Revision: 5933d265b72a8e9aade5edd68998a00dc4fbb359
URL: https://github.com/llvm/llvm-project/commit/5933d265b72a8e9aade5edd68998a00dc4fbb359 DIFF: https://github.com/llvm/llvm-project/commit/5933d265b72a8e9aade5edd68998a00dc4fbb359.diff LOG: [include-cleaner] Add a signal to down-rank exporting headers Currently exporter can have same relevance signals as the origin header when name match signals don't trigger. This patch introduces a tie braker signal to boost origin headers in such cases, this is deliberately introduced with lower significance than public-ness to make sure we still prefer a public-exporter instead of a private-origin header. Differential Revision: https://reviews.llvm.org/D154349 Added: Modified: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp clang-tools-extra/include-cleaner/lib/TypesInternal.h clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp b/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp index 1eff19cb44445b..a1d9d3b5fb2154 100644 --- a/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp +++ b/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp @@ -96,7 +96,7 @@ hintedHeadersForStdHeaders(llvm::ArrayRef<tooling::stdlib::Header> Headers, const SourceManager &SM, const PragmaIncludes *PI) { llvm::SmallVector<Hinted<Header>> Results; for (const auto &H : Headers) { - Results.emplace_back(H, Hints::PublicHeader); + Results.emplace_back(H, Hints::PublicHeader | Hints::OriginHeader); if (!PI) continue; for (const auto *Export : PI->getExporters(H, SM.getFileManager())) @@ -186,10 +186,12 @@ llvm::SmallVector<Hinted<Header>> findHeaders(const SymbolLocation &Loc, if (!FE) return {}; if (!PI) - return {{FE, Hints::PublicHeader}}; + return {{FE, Hints::PublicHeader | Hints::OriginHeader}}; + bool IsOrigin = true; while (FE) { - Hints CurrentHints = isPublicHeader(FE, *PI); - Results.emplace_back(FE, CurrentHints); + Results.emplace_back(FE, + isPublicHeader(FE, *PI) | + (IsOrigin ? Hints::OriginHeader : Hints::None)); // FIXME: compute transitive exporter headers. for (const auto *Export : PI->getExporters(FE, SM.getFileManager())) Results.emplace_back(Export, isPublicHeader(Export, *PI)); @@ -205,6 +207,7 @@ llvm::SmallVector<Hinted<Header>> findHeaders(const SymbolLocation &Loc, // Walkup the include stack for non self-contained headers. FID = SM.getDecomposedIncludedLoc(FID).first; FE = SM.getFileEntryForID(FID); + IsOrigin = false; } return Results; } diff --git a/clang-tools-extra/include-cleaner/lib/TypesInternal.h b/clang-tools-extra/include-cleaner/lib/TypesInternal.h index 09a3933577a94f..e9f8689e8647e0 100644 --- a/clang-tools-extra/include-cleaner/lib/TypesInternal.h +++ b/clang-tools-extra/include-cleaner/lib/TypesInternal.h @@ -63,17 +63,20 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &, const SymbolLocation &); /// Hints are sorted in ascending order of relevance. enum class Hints : uint8_t { None = 0x00, + /// Symbol is directly originating from this header, rather than being + /// exported or included transitively. + OriginHeader = 1 << 0, /// Provides a generally-usable definition for the symbol. (a function decl, /// or class definition and not a forward declaration of a template). - CompleteSymbol = 1 << 0, + CompleteSymbol = 1 << 1, /// Symbol is provided by a public file. Only absent in the cases where file /// is explicitly marked as such, non self-contained or IWYU private /// pragmas. - PublicHeader = 1 << 1, + PublicHeader = 1 << 2, /// Header providing the symbol is explicitly marked as preferred, with an /// IWYU private pragma that points at this provider or header and symbol has /// ~the same name. - PreferredHeader = 1 << 2, + PreferredHeader = 1 << 3, LLVM_MARK_AS_BITMASK_ENUM(PreferredHeader), }; LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE(); @@ -86,6 +89,11 @@ template <typename T> struct Hinted : public T { bool operator<(const Hinted<T> &Other) const { return static_cast<int>(Hint) < static_cast<int>(Other.Hint); } + + friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, + const Hinted<T> &H) { + return OS << static_cast<int>(H.Hint) << " - " << static_cast<T>(H); + } }; } // namespace clang::include_cleaner diff --git a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp index 5b5f77b5fdea80..b6521d56bcff44 100644 --- a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp @@ -422,8 +422,12 @@ TEST(WalkUsed, FilterRefsNotSpelledInMainFile) { } } +struct Tag { + friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Tag &T) { + return OS << "Anon Tag"; + } +}; TEST(Hints, Ordering) { - struct Tag {}; auto Hinted = [](Hints Hints) { return clang::include_cleaner::Hinted<Tag>({}, Hints); }; diff --git a/clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp b/clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp index 964f4c6c9190de..9f9ac11a93eb85 100644 --- a/clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp @@ -8,17 +8,21 @@ #include "AnalysisInternal.h" #include "TypesInternal.h" +#include "clang-include-cleaner/Analysis.h" #include "clang-include-cleaner/Record.h" #include "clang-include-cleaner/Types.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/Basic/FileEntry.h" #include "clang/Basic/FileManager.h" +#include "clang/Basic/LLVM.h" #include "clang/Frontend/FrontendActions.h" #include "clang/Testing/TestAST.h" #include "clang/Tooling/Inclusions/StandardLibrary.h" #include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringRef.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +#include <cassert> #include <memory> namespace clang::include_cleaner { @@ -253,11 +257,11 @@ TEST_F(FindHeadersTest, PublicHeaderHint) { EXPECT_THAT( findHeaders("private.inc"), UnorderedElementsAre( - HintedHeader(physicalHeader("private.inc"), Hints::None), + HintedHeader(physicalHeader("private.inc"), Hints::OriginHeader), HintedHeader(physicalHeader("public.h"), Hints::PublicHeader))); EXPECT_THAT(findHeaders("private.h"), - UnorderedElementsAre( - HintedHeader(physicalHeader("private.h"), Hints::None))); + UnorderedElementsAre(HintedHeader(physicalHeader("private.h"), + Hints::OriginHeader))); } TEST_F(FindHeadersTest, PreferredHeaderHint) { @@ -269,11 +273,12 @@ TEST_F(FindHeadersTest, PreferredHeaderHint) { )cpp"); buildAST(); // Headers explicitly marked should've preferred signal. - EXPECT_THAT(findHeaders("private.h"), - UnorderedElementsAre( - HintedHeader(physicalHeader("private.h"), Hints::None), - HintedHeader(Header("\"public.h\""), - Hints::PreferredHeader | Hints::PublicHeader))); + EXPECT_THAT( + findHeaders("private.h"), + UnorderedElementsAre( + HintedHeader(physicalHeader("private.h"), Hints::OriginHeader), + HintedHeader(Header("\"public.h\""), + Hints::PreferredHeader | Hints::PublicHeader))); } class HeadersForSymbolTest : public FindHeadersTest { @@ -339,11 +344,12 @@ TEST_F(HeadersForSymbolTest, RankByName) { } TEST_F(HeadersForSymbolTest, Ranking) { - // Sorting is done over (canonical, public, complete) triplet. + // Sorting is done over (canonical, public, complete, origin)-tuple. Inputs.Code = R"cpp( #include "private.h" #include "public.h" #include "public_complete.h" + #include "exporter.h" )cpp"; Inputs.ExtraFiles["public.h"] = guard(R"cpp( struct foo; @@ -352,11 +358,15 @@ TEST_F(HeadersForSymbolTest, Ranking) { // IWYU pragma: private, include "canonical.h" struct foo; )cpp"); + Inputs.ExtraFiles["exporter.h"] = guard(R"cpp( + #include "private.h" // IWYU pragma: export + )cpp"); Inputs.ExtraFiles["public_complete.h"] = guard("struct foo {};"); buildAST(); EXPECT_THAT(headersForFoo(), ElementsAre(Header("\"canonical.h\""), physicalHeader("public_complete.h"), physicalHeader("public.h"), + physicalHeader("exporter.h"), physicalHeader("private.h"))); } @@ -424,6 +434,24 @@ TEST_F(HeadersForSymbolTest, PreferExporterOfPrivate) { physicalHeader("private.h"))); } +TEST_F(HeadersForSymbolTest, ExporterIsDownRanked) { + Inputs.Code = R"cpp( + #include "exporter.h" + #include "zoo.h" + )cpp"; + // Deliberately named as zoo to make sure it doesn't get name-match boost and + // also gets lexicographically bigger order than "exporter". + Inputs.ExtraFiles["zoo.h"] = guard(R"cpp( + struct foo {}; + )cpp"); + Inputs.ExtraFiles["exporter.h"] = guard(R"cpp( + #include "zoo.h" // IWYU pragma: export + )cpp"); + buildAST(); + EXPECT_THAT(headersForFoo(), ElementsAre(physicalHeader("zoo.h"), + physicalHeader("exporter.h"))); +} + TEST_F(HeadersForSymbolTest, PreferPublicOverNameMatchOnPrivate) { Inputs.Code = R"cpp( #include "foo.h" @@ -496,6 +524,5 @@ TEST_F(HeadersForSymbolTest, StandardHeaders) { tooling::stdlib::Header::named("<assert.h>"))); } - } // namespace } // namespace clang::include_cleaner _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
