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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Insertion side LGTM, feel free to split and land.
Sorry I need to take off and will need to get to indexing on monday :(



================
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
----------------
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.


================
Comment at: clangd/ClangdServer.cpp:370
+/// File, by matching \p Header against all include search directories for \p
+/// File via `clang::HeaderSearch`.
+///
----------------
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


================
Comment at: clangd/Headers.cpp:74
+
+  // We don't need to provide the content of \p File, as we are only interested
+  // in the include search directories in the compile command.
----------------
comment is outdated now


================
Comment at: clangd/Headers.cpp:114
+
+  log("Suggested #include is: " + Suggested);
+  return Suggested;
----------------
can you include the Header in this log message? (and possibly File, but that might add more noise than signal)


================
Comment at: clangd/Headers.h:30
+/// \return A quoted "path" or <path>. If \p Header is already (directly)
+/// included in the file (including those included via different paths), an
+/// error will be returned.
----------------
header-already-included is not an error condition.

Suggest returning llvm::Expected<Optional<String>>, or returning "" for this case.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42640