[PATCH] D43462: [clangd] Fixes for #include insertion.

[Eric Liu via Phabricator via cfe-commits]

ioeric marked 5 inline comments as done.
ioeric added inline comments.


================
Comment at: clangd/Headers.cpp:60
     Argv.push_back(S.c_str());
   IgnoringDiagConsumer IgnoreDiags;
   auto CI = clang::createInvocationFromCommandLine(
----------------
ilya-biryukov wrote:
> Not related to this patch, but just noticed that we don't call `FS->setCurrentWorkingDirectory(CompileCommand.Directory)` here.
> This might break if `CompileCommand` has non-absolute paths.
This was done in ClangdServer, but we should probably do it here as you suggested.


================
Comment at: clangd/index/FileIndex.cpp:36
+  CollectorOpts.IndexMainFiles = false;
+  CollectorOpts.CollectIncludePath = true;
+  const auto *Includes = CanonicalIncludesForSystemHeaders();
----------------
sammccall wrote:
> if this is always true now, we could remove the option?
Will do this in a followup patch. Added a FIXME


================
Comment at: clangd/index/FileIndex.cpp:37
+  CollectorOpts.CollectIncludePath = true;
+  const auto *Includes = CanonicalIncludesForSystemHeaders();
+  CollectorOpts.Includes = Includes;
----------------
sammccall wrote:
> This is not going to handle IWYU right?
> It seems like at this point the right thing to do is totally hide the CanonicalIncludes inside SymbolCollector, which would init it (to system headers), write IWYU mappings using the PP, and consume the mappings (when emitting symbols).
> Churn :(
You are right :(

I'll prepare a followup patch to handle this. Added a FIXME.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43462