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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
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:
> ioeric wrote:
> > 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.
> I'd still think pulling out `Expected<tooling::Replacement> insertInclude(File, Code, Header, VFS, Style)` would be worthwhile here - the formatting isn't a lot of code, but it's a bit magic, plus the quote handling... it's a bit of code. It'd make it more obvious what the interactions with ClangdServer's state are. But up to you, we can always do this later.
So it's just 2 lines for the replacement magic (+1 for comment) now after removing some redundant code. 

I like `shortenIncludePath` better because it's more self-contained and easier to write tests against, and `insertInclude` doesn't seem to carry much more weight while we would need to handle `replacements` logic which has been tested in the tests.


================
Comment at: clangd/ClangdServer.cpp:370
+/// File, by matching \p Header against all include search directories for \p
+/// File via `clang::HeaderSearch`.
+///
----------------
sammccall wrote:
> sammccall wrote:
> > maybe drop "via clang::HeaderSearch" (it's doc'd in the implementation) and add an example?
> > 
> > It might be worth explicitly stating that the result is an #include string (with quotes) - you just call it a "path" here.
> > 
> > "shortest" makes sense as a first implementation, but we may want to document something more like "best" - I know there are codebases we care about where file-relative paths are banned. Also I suspect we don't give "../../../../Foo.h" even if it's shortest :-)
> > "shortest" makes sense as a first implementation, but we may want to document something more like "best" - I know there are codebases we care about where file-relative paths are banned. Also I suspect we don't give "../../../../Foo.h" even if it's shortest :-)
> 
> I think this part wasn't addressed
I added a comment for this in the header. But I might be misunderstanding you suggestion. Did you mean we need a better name for the function?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42640