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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Some comments on the insert side, which looks pretty good. I'll take another look at indexing tomorrow.



================
Comment at: clangd/ClangdServer.cpp:465
+    auto &HeaderSearchInfo = Clang->getPreprocessor().getHeaderSearchInfo();
+    std::string Suggested = HeaderSearchInfo.suggestPathToFileForDiagnostics(
+        *Resolved, CompileCommand.Directory);
----------------
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.


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


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


================
Comment at: clangd/ClangdServer.cpp:379
+                                 llvm::StringRef Header) {
+  // In order to get a HeaderSearch instance, we first create a CompilerInstance
+  // and initialize it to get a Preprocessor which provides HeaderSearch.
----------------
This comment helps a lot. The subtext is: HeaderSearch is hard to construct directly, so we're doing this weird dance.
I think this is worth calling out even louder - when I read this sort of code I tend to take a *long* time to work out why the code seems to be doing unrelated work.


================
Comment at: clangd/ClangdServer.cpp:420
+  auto &HeaderSearchInfo = Clang->getPreprocessor().getHeaderSearchInfo();
+  std::string Suggested = HeaderSearchInfo.suggestPathToFileForDiagnostics(
+      Header, CompileCommand.Directory);
----------------
consume the optional `IsSystem` and use it to quote appropriately?


================
Comment at: unittests/clangd/ClangdTests.cpp:849
 
+TEST_F(ClangdVFSTest, InsertIncludes) {
+  MockFSProvider FS;
----------------
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)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42640