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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thanks for the comments!

Sorry that I didn't clean up the code before sending out the prototype. I planned to deal with code structure and style issues after getting some early feedback, but I think the patch is ready for review now.



================
Comment at: clangd/ClangdServer.cpp:445
+    CI->getFrontendOpts().DisableFree = false;
+    CI->getPreprocessorOpts().SingleFileParseMode = true;
+
----------------
sammccall wrote:
> explain why? (this has implications for direct vs transitive includes I think)
Since we only need predecessor for HeaderSearch and don't really care about the actual code, I set this to false in the hope of speeding up the code. But in the latest revision, I simply use an empty file (as we only care about header search), so this option is no longer necessary. 


================
Comment at: clangd/ClangdServer.cpp:447
+
+    auto Clang = prepareCompilerInstance(
+        std::move(CI), /*Preamble=*/nullptr,
----------------
sammccall wrote:
> hmm, why are we actually going to run the compiler/preprocessor?
> It looks like we just get HeaderMapping out - can we "just" call ApplyHeaderSearchOptions instead?
I couldn't find an easy way to use `ApplyHeaderSearchOptions`... It requires an instance of HeaderSearch, which needs a preprocessor and a bunch of other objects (SourceManager, Target etc). And these objects are mostly initialized in `BeginSourceFile`. We could probably pull out the code that only initializes up to proprocessor,  but this is not very trivial :(


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


================
Comment at: clangd/CodeComplete.cpp:836
                        if (Recorder.CCSema)
-                         Output = runWithSema();
+                         Output = runWithSema(SemaCCInput.FileName,
+                                              SemaCCInput.Contents);
----------------
sammccall wrote:
> What do you think about (redundantly) passing filename to the constructor, and stashing it in a member, instead of passing to all the methods here?
> I think main purpose of having this class is not having the *primary* data flow obscured by all the various context variables.
> 
> If you don't like that, we could also pass the SemaCCInput struct to the constructor itself. The distinction between constrcutor params and run() params is pretty artificial.
> 
> (as noted above, I think/hope Contents is unused in any case)
Passing filename to the constructor sounds good.  Thanks!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42640