hokein added inline comments.
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:58
+ Results = {{FE}};
+ // FIXME: compute a closure of exporter headers.
+ for (const auto *Export : PI.getExporters(FE, SM.getFileManager()))
----------------
kadircet wrote:
> hokein wrote:
> > kadircet wrote:
> > > not sure i follow the fixme here, are you suggesting we should also
> > > compute transitive exporters?
> > Yeah, rephrased the FIXME.
> i see, i am not sure if that's desired though.
>
> e.g. if we have `foo.h` exporting `bar.h` exporting `baz.h`, I don't think
> `baz.h` counts (or should count) as an alternative to `foo.h`. do we have
> information proving otherwise?
The classical IWYU tool has this semantic, see the nested_export test in
https://github.com/include-what-you-use/include-what-you-use/commit/9945e543d894e90fab5ebd0b685cabd5aa8dd21f.
================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:135
+
+TEST(FindIncludeHeaders, IWYU) {
+ TestInputs Inputs;
----------------
kadircet wrote:
> you can still use the same fixture here, if you want a different name you can
> always do: `using FindIncludeHeaders = WalkUsedTest;` or `class
> FindIncludeHeaders : public WalkUsedTest {};`
There is not much code being shared between these two tests (only the
MakeAction, walk() is not used) -- the FindIncludeHeaders test is simpler than
the WalkUsedTest, we don't need to build an AST or traverse the AST nodes.
================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:116
+
+ EXPECT_THAT(walk(AST),
+ UnorderedElementsAre(
----------------
kadircet wrote:
> hokein wrote:
> > kadircet wrote:
> > > can we use `findIncludeHeaders` directly?
> > >
> > > that way we don't need to set up a referencing file, e.g. you can
> > > directly create a SourceLocation inside `private.h` (in this case even
> > > the decl is not important).
> > >
> > >
> > > (same for the export test)
> > the intention was to do an end-to-end test of the walkUsed API.
> >
> > but you're right, we should have a unittest for `findIncludeHeaders`, added
> > one, and simplified the end-to-end walkUsed test.
> > the intention was to do an end-to-end test of the walkUsed API.
>
> We can update the `Basic` test above for that purpose (and turn it into a
> smoke test instead).
>
> > but you're right, we should have a unittest for findIncludeHeaders, added
> > one, and simplified the end-to-end walkUsed test.
>
> Since these are all trying to test specific IWYU pragma behaviour, I still
> think it's better to test them in isolation, both for test brevity but also
> for debuggability in the future when something breaks.
> Also making sure majority of the tests are smaller is good for incremental
> progress as well, when we update ast walking logic, we shouldn't be updating
> these tests for whatever reasons (+ arguments around test times).
ok, moved to WalkUsedTest.Basic. now the test is just a smoke test for IWYU
pragma.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137320/new/
https://reviews.llvm.org/D137320
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
