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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D46497#1089755, @sammccall wrote:

> I'm concerned about the scope of this patch - it does too many things and touches too many files for me to feel comfortable that I understand it well enough to review.
>  Is it possible to split it up? (You mention 6 distinct things in the description).


Sorry about the big change! And thanks for the early feedback on headers library!

I think there are a few things going on here:

1. Removing the existing way of include insertion (additional command)
2. Populating #include insertion in code completion workflow.
3. Refactoring #include path calculation to make 2 work
4. Move some helpers (e.g. replacementToEdit) to public header.
5. Share InclusionLocation between preamble code and header insertion code.

I was trying to keep 1 and 2 in the same patch so that we don't regress features, but it's probably fine if no one is actually using it... I'm happy to split that out if having them in the same patch makes review hard.

I think 2 and 3 could stay in the same patch as it would be hard to see where the design in 3 came from without 2. But if you got the idea, I guess they could still be split.

4 and 5 should be independent enough to be 2 separate patches. I thought the changes were small and could be included in this patch. I'll split them as well now that you mention.



================
Comment at: clangd/Headers.h:1
 //===--- Headers.h - Include headers -----------------------------*- C++-*-===//
 //
----------------
sammccall wrote:
> The interface to this header looks a bit complex and entangled after this change.
> - The construction/lifecycle of IncludePaths is confusing (comments below)
> - here we provide all the information needed to compute the edits (style, code...) but don't actually do so
> 
> it would be nice to achieve a simpler interface with the restricted set of functionality, or expand the functionality...
Refactored code according to your suggestions. PTAL

> here we provide all the information needed to compute the edits (style, code...) but don't actually do so
Sorry... forgot to delete some leftover after an earlier revision. 


================
Comment at: clangd/Headers.h:29
 
+// A header inclusion in the main file.
+struct InclusionLocation {
----------------
sammccall wrote:
> I had some trouble working out the role of this struct (e.g. that it might have been a *proposed* inclusion).
> Strictly it's not just the location, but also identifies the header being included.
> 
> So this could maybe be
> ```
> // An #include directive that we found in the main file.
> struct Inclusion
> ```
> 
> but it does also seem a little strange that this is part of the public interface at all (other than for testing)
Thanks! `Inclusion` sounds better.

Apart from header insertion and testing, this is also shared with the preamble code.


================
Comment at: clangd/Headers.h:57
+/// preprocessor and build configs.
+class IncludePaths {
+public:
----------------
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.


================
Comment at: clangd/Headers.h:60
+  /// This must be called before preprocessor is run (this registers a
+  /// PPCallback on \p PP).
+  /// \p SM is used in the PPCallbacks to filter source locations in main file.
----------------
sammccall wrote:
> hmm, if we require the InclusionLocations to be prebuilt for the preamble, why can't we do the same for non-preamble includes? i.e. can't we split up the "collect existing includes" and "calculate new includes" parts to simplify the interface here? Among other things, testing would be a ton easier.
Good idea! 


================
Comment at: clangd/Headers.h:95
+  std::shared_ptr<Preprocessor> PP;
+  std::unique_ptr<tooling::HeaderIncludes> Inserter;
+};
----------------
sammccall wrote:
> is this unused? this seems like it could be a natural part of this class...
Thanks for the catches! Missed these after iterating from an earlier version.,,


================
Comment at: clangd/tool/ClangdMain.cpp:103
                    "0 means no limit."),
-    llvm::cl::init(100));
+    llvm::cl::init(1000));
 
----------------
sammccall wrote:
> why this change?
Oops, this was not intended. I increased the limit for profiling. Reverted...


================
Comment at: unittests/clangd/HeadersTests.cpp:48
+    IgnoringDiagConsumer IgnoreDiags;
+    auto CI = clang::createInvocationFromCommandLine(
+        Argv,
----------------
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...


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46497