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!

  rCTE Clang Tools Extra

cfe-commits mailing list

Reply via email to