[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thanks! PTAL



================
Comment at: clangd/ClangdServer.cpp:465
+    auto &HeaderSearchInfo = Clang->getPreprocessor().getHeaderSearchInfo();
+    std::string Suggested = HeaderSearchInfo.suggestPathToFileForDiagnostics(
+        *Resolved, CompileCommand.Directory);
----------------
sammccall wrote:
> ioeric wrote:
> > sammccall wrote:
> > > do we handle the case that the suggestion is already included?
> > > (including the case where it's included by a different name)
> > Yes and no. The current implementation only does textual matching against existing #includes in the current file and inserts the header if no same header was found. This complies with IWYU. But we are not handling the case where the same header is included by different names. I added a `FIXME` for this.
> I can't see the FIXME? (There's one in the header, but it doesn't seem to really cover this case)
> 
> So this doesn't seem so hard: we can pass the file content, turn off recursive PP, and add PP callbacks to capture the names of each included file (the include-scanner patch does this).
> I'm not sure it's worth deferring, at least we should fix it soon before we lose context.
> 
> But up to you, I'd suggest putting the fixme where we expect to fix it.
Since you prefer, I have included the change in the patch. I wanted to get to this as soon as this patch is landed. 


================
Comment at: clangd/ClangdServer.cpp:368
 
+/// Calculates the shortest possible include path when inserting \p Header to \p
+/// File, by matching \p Header against all include search directories for \p
----------------
sammccall wrote:
> This has a fair amount of logic (well, plumbing :-D) and uses relatively little from ClangdServer.
> Can we move `shortenIncludePath` to a separate file and pass in the FS and command?
> 
> I'd suggest maybe `Headers.h` - The formatting part of `insertInclude` could also fit there, as well as probably the logic from `switchSourceHeader` I think (the latter not in this patch of course).
> 
> This won't really help much with testing, I just find it gets hard to navigate files that have lots of details of unrelated features. ClangdUnit and ClangdServer both have this potential to grow without bound, though they're in reasonable shape now. Interested what you think!
Done.

I didn't move the formatting code, as half of the code is pulling out the style, which we might want to share/change depending on other clangd logics that might use style.


================
Comment at: unittests/clangd/ClangdTests.cpp:849
 
+TEST_F(ClangdVFSTest, InsertIncludes) {
+  MockFSProvider FS;
----------------
sammccall wrote:
> Similarly, it'd be nice to pull these tests out into a test file parallel to the header.
> (As with the other tests, often it's easiest to actually test through the ClangdServer interface - this is mostly just for navigation)
Done. I left a simple test for replacements.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42640