[PATCH] D137698: [include-cleaner] Add self-contained file support for PragmaIncludes.

[Haojian Wu via Phabricator via cfe-commits]

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