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;
> 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,
> 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);
> 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
- Output = runWithSema();
+ Output = runWithSema(SemaCCInput.FileName,
> 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