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