[PATCH] D46497: [clangd] Populate #include insertions as additional edits in completion items.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/CodeComplete.cpp:247
+// Calculates include path and insertion edit for a new header.
+class IncludeInserter {
+public:
----------------
Do you think it would make sense to move this class to Headers.h? This would reduce the non-completion related code in this large file.


================
Comment at: clangd/CodeComplete.cpp:261
+  // The header name is either a URI or a literal include.
+  Expected<Optional<TextEdit>> insert(StringRef FileName,
+                                      StringRef DeclaringHeader,
----------------
If we wanted to avoid URI dependencies in Headers.h but move this class there, the Declaring/Inserted headers could be HeaderFile (possibly invalid). toHeaderFile would stay here.

This might be a neater separation of concerns anyway.


================
Comment at: clangd/CodeComplete.cpp:365
+        if (Includes && !D->IncludeHeader.empty()) {
+          // Fallback to canonical header if declaration location is invalid.
+          llvm::StringRef DeclaringHeader =
----------------
This strikes me as overly defensive. We should never have an include header but no declaration location, and if the declaration header is invalid then the include inserter should take care of that, not this code (I think?)


================
Comment at: clangd/CodeComplete.cpp:744
   PrecompiledPreamble const *Preamble;
+  const std::vector<Inclusion> &Inclusions;
   StringRef Contents;
----------------
PreambleInclusions?


================
Comment at: clangd/CodeComplete.cpp:977
+                     SemaCCInput,
+                     /*CallbackBeforeAction*/ [&](CompilerInstance &Clang) {
+                       Clang.getPreprocessor().addPPCallbacks(
----------------
this callback is a bit confusing, particularly sequencing/lifetimes.
semaCodeComplete is local, could we simplify it by coupling a bit, passing an `IncludeInserter*` ("will be populated by the preprocessor") and moving this code (and the preamble-includes handling) inside semaCodeComplete?

This would be a bit more natural if we had `IncludeInsertion::addExisting(Insertion)` instead of `IncludeInsertion::setExisting(vector<Insertion>)`, I think.


================
Comment at: clangd/CodeComplete.cpp:1142
+      {FileName, Command, Preamble,
+       /*Inclusions*/ {}, Contents, Pos, std::move(VFS), std::move(PCHs)});
   return Result;
----------------
nit: `/*Inclusions=*/`


================
Comment at: clangd/CodeComplete.h:73
                             PrecompiledPreamble const *Preamble,
+                            const std::vector<Inclusion> &Inclusions,
                             StringRef Contents, Position Pos,
----------------
should this be PreambleInclusions, or is it broader than that?


================
Comment at: clangd/Headers.h:57
+/// preprocessor and build configs.
+class IncludePaths {
+public:
----------------
ioeric wrote:
> sammccall wrote:
> > This name suggests a set or sequence of include path entries (`-I` flags).
> > Can we merge this with `IncludeInsertion` and call it `IncludeInserter` or similar?
> I think there are three pretty independent parts of include insertion:
> 1. URI/Verbatim to `HeaderFile` conversion (which lives in HeaderInsertion in CodeComplete.cpp right now).
> 2. Calculate include paths based on existing inclusions and HeaderSearch.
> 3. Calculate include insertion edits using `tooling::HeaderIncludes`.
> 
> Initially, I grouped all these in one class (something like `HeaderInsertion`) which returns an edit given the URIs in the symbol info, but it turned out to be hard to test. For example, among all 3 components, 2 was the most interesting piece to test, while an integration test is probably enough for 1 and 3, but we would need to create URIs (due to 1) and retrieve include name from edits (due to 3) in order to test 2. Since each individual piece seemed independent enough, I decided to keep them separated. I also kept URI logic out of the header library so that it could potentially be shared with other features that are not index-based include insertion, or be moved out of clangd as a tooling library. 
> 
> In the new revision, I refactored the code according to your suggestions and got rid of this class.
OK, SGTM


================
Comment at: unittests/clangd/HeadersTests.cpp:48
+    IgnoringDiagConsumer IgnoreDiags;
+    auto CI = clang::createInvocationFromCommandLine(
+        Argv,
----------------
ioeric wrote:
> sammccall wrote:
> > Is there any way we can avoid creating another copy of this code?
> This was moved here. Headers library no longer needs compiler instance. I'm not sure if there is an easier way to test `HeaderSearch` and `PPCallbacks` without a compiler instance...
Moved from where?

You could consider adding inclusions to TestTU (sorry it's in D46524 which I thought had already landed, but should today)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46497