sammccall marked an inline comment as done.
sammccall added inline comments.
================
Comment at: clang-tools-extra/include-cleaner/lib/HTMLReport.cpp:116
FileID File;
+ const FileEntry *FE;
----------------
hokein wrote:
> nit: the `FE` can be removed -- we have the FileID, we can retrieve it by
> `SM.getFileEntryByID(File)`
Sure, I'd just rather do that in the constructor than in the inner loop
================
Comment at: clang-tools-extra/include-cleaner/lib/HTMLReport.cpp:160
+
+ for (const auto &H : T.Headers) {
+ T.Includes.append(Includes.match(H));
----------------
hokein wrote:
> The current behaivor looks fine to me, but this is not the final state,
> right? We will revisit it when we have all the ranking/included-header bits
> in the library, if so, probably add a FIXME?
Ranking isn't relevant whether a ref is satisfied or not.
If we add the concept of one provider dominating another, this logic might
change. (e.g. for `std::vector` accept `<iosfwd>` but not if `<vector>` is
included). But this is up in the air and also would probably be encapsulated in
a signature change to `Includes.match`.
Added a FIXME to avoid the brittle main-file check.
================
Comment at: clang-tools-extra/include-cleaner/lib/HTMLReport.cpp:162
+ T.Includes.append(Includes.match(H));
+ if (H.kind() == Header::Physical && H.physical() == FE)
+ T.Satisfied = true;
----------------
hokein wrote:
> a comment -- this is for the target symbol defined in the main file?
Defined or declared or otherwise provided by the main file.
Renamed FE => MainFE. Beyond that I think such a comment just echoes the code.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138219/new/
https://reviews.llvm.org/D138219
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
