Skip to content

Commit

Permalink
[include-cleaner] Weaken signal for boosting preferred headers
Browse files Browse the repository at this point in the history
Putting preferred header signal above completeness implied we would
uprank forward declarations above complete ones in certain cases.

This can be desired in cases where:
- Complete definition is private. But this case is already governed by
  publicness signal.
- The library indeed intends to provide a forward declaring interface, like
  iosfwd vs ostream.

In all other cases, upranking is undesired as it means we've picked up prefered
headerness signal by mistake from an unrelated declaration to the library.

This change regresses the behavior for libraries that intentionally provide a
forward declaring interface. But that wasn't something we intended to support
explicitly, it was working incidentally when the forward declaring header had a
similar name to the symbol. Moreover, include-cleaner deliberately discourages
forward-declarations, so not working in this case is also more aligned with rest
of the components.

Differential Revision: https://reviews.llvm.org/D159441
  • Loading branch information
kadircet committed Sep 6, 2023
1 parent b93d2d3 commit 73b2c86
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 16 deletions.
8 changes: 4 additions & 4 deletions clang-tools-extra/include-cleaner/lib/TypesInternal.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,13 @@ enum class Hints : uint8_t {
/// 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 << 1,
/// 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 << 1,
/// Provides a generally-usable definition for the symbol. (a function decl,
/// or class definition and not a forward declaration of a template).
CompleteSymbol = 1 << 2,
/// 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.
Expand Down
21 changes: 9 additions & 12 deletions clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ TEST_F(HeadersForSymbolTest, RankByName) {
}

TEST_F(HeadersForSymbolTest, Ranking) {
// Sorting is done over (canonical, public, complete, origin)-tuple.
// Sorting is done over (public, complete, canonical, origin)-tuple.
Inputs.Code = R"cpp(
#include "private.h"
#include "public.h"
Expand All @@ -363,11 +363,11 @@ TEST_F(HeadersForSymbolTest, Ranking) {
)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")));
EXPECT_THAT(headersForFoo(),
ElementsAre(physicalHeader("public_complete.h"),
Header("\"canonical.h\""), physicalHeader("public.h"),
physicalHeader("exporter.h"),
physicalHeader("private.h")));
}

TEST_F(HeadersForSymbolTest, PreferPublicOverComplete) {
Expand All @@ -391,14 +391,11 @@ TEST_F(HeadersForSymbolTest, PreferNameMatch) {
#include "public_complete.h"
#include "test/foo.fwd.h"
)cpp";
Inputs.ExtraFiles["public_complete.h"] = guard(R"cpp(
struct foo {};
)cpp");
Inputs.ExtraFiles["public_complete.h"] = guard("struct foo {};");
Inputs.ExtraFiles["test/foo.fwd.h"] = guard("struct foo;");
buildAST();
EXPECT_THAT(headersForFoo(),
ElementsAre(physicalHeader("test/foo.fwd.h"),
physicalHeader("public_complete.h")));
EXPECT_THAT(headersForFoo(), ElementsAre(physicalHeader("public_complete.h"),
physicalHeader("test/foo.fwd.h")));
}

TEST_F(HeadersForSymbolTest, MainFile) {
Expand Down

0 comments on commit 73b2c86

Please sign in to comment.