VitaNuo marked an inline comment as done.
VitaNuo added a comment.
Thank you for all the thoughtful comments!
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:414
+
+std::string findResolvedPath(const include_cleaner::Header &SymProvider) {
+ std::string ResolvedPath;
----------------
kadircet wrote:
> what about just `resolvedPath`, if you'd rather keep the verb, i think `get`
> makes more sense than `find`. we're not really searching anything.
Ok, let's call it `get`. I do prefer verbs for methods, that's correct.
================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:449
+ include_cleaner::Symbol B{*D};
+ syntax::FileRange BRange{SM, B.declaration().getBeginLoc(), 1};
+ include_cleaner::Header Header{*SM.getFileManager().getFile("b.h")};
----------------
kadircet wrote:
> this is pointing at the declaration inside `b.h` not to the reference inside
> the main file. are you sure this test passes?
Yes, all the tests pass.
`D` is a `Decl` from the main file, otherwise it wouldn't have passed the
safeguard ` if (!SM.isWrittenInMainFile(SM.getExpansionLoc(D->getLocation())))
continue;` above.
================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:469
+ $d[[d]]();
+ $f[[f]]();
+ })cpp");
----------------
kadircet wrote:
> can you also add a reference (and declaration) for std::vector, and have an
> IWYU private pragma in one of the headers to test code paths that spell
> verbatim and standard headers? also having some diagnostic suppressed via
> `IgnoreHeaders` is important to check
Thank you for the great tips on improving test coverage!
In fact, I had to also introduce support for private pragmas, as they were not
taken care of. Hopefully, the solution will make sense to you.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143496/new/
https://reviews.llvm.org/D143496
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
