[PATCH] D139921: [include-cleaner] Ranking of providers based on hints

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

I haven't read all code yet, left a few initial comments.



================
Comment at: clang-tools-extra/include-cleaner/lib/AnalysisInternal.h:85
+/// Represents properties of a symbol provider.
+enum class Hint : uint8_t {
+  None = 0x00,
----------------
IIUC, we're using a union bit for symbols and headers: some properties are for both (e.g. Complete); some properties are just for headers (e.g. Public, Canonical).

I found that this model is hard to follow (e.g. for each property, I need to keep thinking about whether this property is for just symbols/headers or both, the hint conversion step from Symbol to Header is not obvious).

What do you think about the following alternative ?

- define two hints (`SymbolHint`, `HeaderHint`)
- an explicit conversion from `SymbolHint` to `HeaderHint`
- `SymbolLocation` class has a `SymbolHint` member (for `Header` as well)






================
Comment at: clang-tools-extra/include-cleaner/lib/AnalysisInternal.h:89
+  /// difference. e.g. types or templated functions/classes.
+  Complete = 0x01,
+  /// Provides a non-private declaration for the symbol. Only absent in the
----------------
nit: since this is a bit union, I'd suggest to use `= 1 << 0`, `= 1 << 1` syntax to define the value.


================
Comment at: clang-tools-extra/include-cleaner/lib/AnalysisInternal.h:90
+  Complete = 0x01,
+  /// Provides a non-private declaration for the symbol. Only absent in the
+  /// cases where file is explicitly marked as such, e.g. non self-contained or
----------------
I think this is no non-private declaration. The `Public` property is for headers, it indicates the symbol is provided by a non-private header.


================
Comment at: clang-tools-extra/include-cleaner/lib/AnalysisInternal.h:93
+  /// IWYU private pragmas.
+  Public = 0x02,
+  /// Declaration is explicitly marked as canonical, e.g. with a IWYU private
----------------
In most cases, this bit is set. If we use a flipped bit (`Private`), we don't need to set it in many places.


================
Comment at: clang-tools-extra/include-cleaner/lib/AnalysisInternal.h:95
+  /// Declaration is explicitly marked as canonical, e.g. with a IWYU private
+  /// pragma that points at this provider.
+  Canonical = 0x04,
----------------
This comment is hard to follow, it doesn't explain the `Canonical`. From the design doc, it means `NameMatch` and `Annotated` headers, I think it would be nice to document them in the comment.

I'd suggest avoid using the `Canonical` name, it remains me too much about the canonical decl in clang. Maybe `Preferred`?


================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:1
 //===--- FindHeaders.cpp --------------------------------------------------===//
 //
----------------
any reason to change the file property?


================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:97
       for (const auto *Export : PI->getExporters(FE, SM.getFileManager()))
-        Results.push_back(Header(Export));
+        Results.emplace_back(Export, CurrentHints);
 
----------------
I think `Exporter` is the case we need to handle -- the exporter header should have a higher value (we probably need to add another hint bit).


================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:100
+      if (auto Verbatim = PI->getPublic(FE); !Verbatim.empty()) {
+        Results.emplace_back(Verbatim, CurrentHints | Hint::Canonical);
         break;
----------------

if FE is `private`, do we want  the `Verbatim` set the public bit? My inclination is yes (the current code behavior is no), it feels counterintuitive that  headers from `getPublic()` is not marked as `Public`.


================
Comment at: clang-tools-extra/include-cleaner/lib/LocateSymbol.cpp:25
 
-std::vector<SymbolLocation> locateDecl(const Decl &D) {
-  std::vector<SymbolLocation> Result;
+Hint hints(const Decl *D) {
+  Hint Hints = Hint::None;
----------------
nit: declHints?


================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:161
 
+class HeadersForSymbolTest : public FindHeadersTest {
+protected:
----------------
I think we should have some tests for the hints bit (verify the hint bits are set correctly).


================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:266
+  EXPECT_THAT(headersForFoo(),
+              ElementsAre(physicalHeader("test/foo.proto.h"),
+                          physicalHeader("public_complete.h")));
----------------
The result is correct according to the `(canonical, public, complete)` triple order.

But we can come up with a counterexample, e.g. 
 
```
// transitive.h
#include "public_complete.h"

// main.cpp
#include "transitive.h"
Foo foo;
```

We will remove the `transitive.h` and insert the `foo.proto.h`, then we cause a compiling error (incomplete type of `Foo`). Not sure what we should do about it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139921/new/

https://reviews.llvm.org/D139921