[PATCH] D143496: [clangd] Add support for missing includes analysis.

[Viktoriia Bakalova via Phabricator via cfe-commits]

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