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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks, mostly LG a bunch of suggestions for tests



================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:74
+  /// Returns true if the given file is a self-contained file.
+  bool isSelfContained(const FileEntry *File) const;
+
----------------
we also need tests for this


================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:24
+    const FileEntry *FE = SM.getFileEntryForID(FID);
+    if (!PI && FE)
+      return {Header(FE)};
----------------
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).


================
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);
----------------
nit: `Results.append(PI->getExporters(FE, SM.getFileManager()))`


================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:85
 
   EXPECT_THAT(findHeaders(SourceLocFromFile("header1.h"), SM, &PI),
+              UnorderedElementsAre(Header("\"path/public.h\""),
----------------
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


================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:95
+                                   PhysicalHeader("detail2.h")));
+  // Verify that we emit the exporter for the details3.h.
+  EXPECT_THAT(findHeaders(SourceLocFromFile("imp3.inc"), SM, &PI),
----------------
`Verify that we emit exporters for each header on the path.`


================
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"),
----------------
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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137698/new/

https://reviews.llvm.org/D137698