hokein added inline comments.
================
Comment at: clang-tools-extra/include-cleaner/lib/CMakeLists.txt:12
LINK_LIBS
clangAST
----------------
mstorsjo wrote:
> Note that this file changed a bit further in
> 68294afa0836bb62be921e2143d147cdfdc8ba70, so you may want to rebase again.
>
> (I tested this patch after rebasing, in a mingw+dylib build configuration,
> and it still builds correctly there.)
sure, and thank you very much for taking care of the mingw+dylib build!
================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:24
+ const FileEntry *FE = SM.getFileEntryForID(FID);
+ if (!PI && FE)
+ return {Header(FE)};
----------------
kadircet wrote:
> can we rather write this as:
> ```
> if (!PI)
> return FE ? {Header(FE)} : {};
> ```
>
> i think it makes it more clear that in the absence of PI we're just
> terminating the traversal no matter what (rather than getting into the code
> below with possibly a null PI, because FE also happened to be null).
I agree, we can do that, but but the sad bit is that we need to explicitly
spell the return type in the conditional operator.
================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:30
// FIXME: compute transitive exporter headers.
for (const auto *Export : PI->getExporters(FE, SM.getFileManager()))
Results.push_back(Export);
----------------
kadircet wrote:
> nit: `Results.append(PI->getExporters(FE, SM.getFileManager()))`
the type of two container is different (Header vs FileEntry, I updated to
explicitly spell the `Header` type in the push_back) -- if we want to get rid
of the for-loop, we could use the `llvm::for_each`.
================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:85
EXPECT_THAT(findHeaders(SourceLocFromFile("header1.h"), SM, &PI),
+ UnorderedElementsAre(Header("\"path/public.h\""),
----------------
kadircet wrote:
> tests and assertions here are getting a little bit hard to follow. do you
> mind splitting them into smaller chunks instead? each testing different parts
> of the traversal behaviour. one for private->public mappings, another for
> exports, another for non-self contained traversal. having another big
> integration test for testing each might also be fine, but it's really hard to
> reason about so i don't think that should be the main test in which we're
> testing the behaviour.
>
> apart from that this might be easier to follow if we did some renaming:
> - header1.h -> private1.h
> - header2.h -> exporter.h (also probably move non-exported includes to a
> different header)
> - detailx.h -> exportedx.h
> - impx.inc -> fragmentx.inc
that sounds fair enough, split into small tests.
================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:105
+ // that we don't emit its includer.
+ EXPECT_THAT(findHeaders(SourceLocFromFile("imp_private.inc"), SM, &PI),
+ UnorderedElementsAre(PhysicalHeader("imp_private.inc"),
----------------
kadircet wrote:
> i think it would be better to check we stop traversal once we hit the pragma
> (i.e. have a non-self contained file included by another non-self contained
> file that also has a IWYU private mapping).
I think this is covered by this case (though this case is quite simple) -- we
started the traversing from the imp_private.inc, at the beginning we hit the
the IWYU private mapping, we stop immediately.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137698/new/
https://reviews.llvm.org/D137698
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
