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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
