hokein added a comment.
In D138219#3946020 <https://reviews.llvm.org/D138219#3946020>, @sammccall wrote:
> (I do think the outstanding issues are not related to this patch, so this is
> ready for review)
Agree (sorry, I didn't mean we should address in this patch). We have found a
few issues, I think we should record them in the issue tracker so that we don't
lose them.
================
Comment at: clang-tools-extra/include-cleaner/lib/HTMLReport.cpp:116
FileID File;
+ const FileEntry *FE;
----------------
nit: the `FE` can be removed -- we have the FileID, we can retrieve it by
`SM.getFileEntryByID(File)`
================
Comment at: clang-tools-extra/include-cleaner/lib/HTMLReport.cpp:126
+ SmallVector<const Include *> Includes;
+ bool Satisfied = false;
};
----------------
Worth to add a comment on the meaning of `Satisfied`.
================
Comment at: clang-tools-extra/include-cleaner/lib/HTMLReport.cpp:160
+
+ for (const auto &H : T.Headers) {
+ T.Includes.append(Includes.match(H));
----------------
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?
================
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;
----------------
a comment -- this is for the target symbol defined in the main file?
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
