kadircet added inline comments.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:54
+ /// A verbatim header spelling, a string quoted with <> or "" that can be
+ /// #included directly
+ Header(StringRef VerbatimSpelling) : Storage(VerbatimSpelling) {}
----------------
nit: `.` at the end of comment
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:67
+ }
+ return Results;
+}
----------------
can you actually put an `llvm_unreachable` here?
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:55
+ if (!VerbatimSpelling.empty())
+ return {{VerbatimSpelling}};
+
----------------
hokein wrote:
> kadircet wrote:
> > what about exporters here?
> based on our previous discussion, we decided to initially treat the spelling
> header in the IWYU private pragma as the final public header, so no need to
> do the exporters here.
yeah SG, can you put a comment about that though? saying `// Note that we might
need to look for exporters in this case as well.`
================
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()))
----------------
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?
================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:135
+
+TEST(FindIncludeHeaders, IWYU) {
+ TestInputs Inputs;
----------------
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 {};`
================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:116
+
+ EXPECT_THAT(walk(AST),
+ UnorderedElementsAre(
----------------
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).
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
