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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
