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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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).

Have tried to focus comments on what seems to be the core parts.



================
Comment at: clangd/CodeComplete.cpp:247
+// Calculates include path and insertion edit for a new header.
+class IncludeInsertion {
+public:
----------------
I can't parse this name as it relates to this class. This inserts includes, it isn't itself an insertion.
Maybe `IncludeInserter`?


================
Comment at: clangd/CodeComplete.h:72
+    PathRef FileName, const tooling::CompileCommand &Command,
+    PrecompiledPreamble const *Preamble, const InclusionLocations &IncLocations,
+    StringRef Contents, Position Pos, IntrusiveRefCntPtr<vfs::FileSystem> VFS,
----------------
IncLocations isn't an appropriate name for the variable - it's coupled to the preamble


================
Comment at: clangd/Headers.h:1
 //===--- Headers.h - Include headers -----------------------------*- C++-*-===//
 //
----------------
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...


================
Comment at: clangd/Headers.h:29
 
+// A header inclusion in the main file.
+struct InclusionLocation {
----------------
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)


================
Comment at: clangd/Headers.h:40
+
+using InclusionLocations = std::vector<InclusionLocation>;
+
----------------
This doesn't seem to add any semantics over the vector, so it just seems to obscure a bit.
In fact it does have semantics: it's the stored list of inclusion locations for the preamble. But those semantics don't belong in this layer. I'd suggest moving it back to the preamble code, or avoiding the typedef altogether.


================
Comment at: clangd/Headers.h:55
 
-/// Determines the preferred way to #include a file, taking into account the
-/// search path. Usually this will prefer a shorter representation like
-/// 'Foo/Bar.h' over a longer one like 'Baz/include/Foo/Bar.h'.
-///
-/// \param File is an absolute file path.
-/// \param DeclaringHeader is the original header corresponding to \p
-/// InsertedHeader e.g. the header that declares a symbol.
-/// \param InsertedHeader The preferred header to be inserted. This could be the
-/// same as DeclaringHeader but must be provided.
-//  \return A quoted "path" or <path>. This returns an empty string if:
-///   - Either \p DeclaringHeader or \p InsertedHeader is already (directly)
-///   included in the file (including those included via different paths).
-///   - \p DeclaringHeader or \p InsertedHeader is the same as \p File.
-llvm::Expected<std::string>
-calculateIncludePath(PathRef File, llvm::StringRef Code,
-                     const HeaderFile &DeclaringHeader,
-                     const HeaderFile &InsertedHeader,
-                     const tooling::CompileCommand &CompileCommand,
-                     IntrusiveRefCntPtr<vfs::FileSystem> FS);
+/// Calculates #include paths for inserting new headers into a file, based on
+/// preprocessor and build configs.
----------------
the main value-add of this class over HeaderSearch is that it determines whether to insert, so that should probably be mentioned.


================
Comment at: clangd/Headers.h:56
+/// Calculates #include paths for inserting new headers into a file, based on
+/// preprocessor and build configs.
+class IncludePaths {
----------------
can you describe the design a little bit here?
e.g. "We simply keep track of which files were already inserted, and delegate to clang's HeaderSearch if a header is new"


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


================
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.
----------------
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.


================
Comment at: clangd/Headers.h:63
+  /// \p IncLocations Known inclusions in the main file e.g. from preamble.
+  IncludePaths(StringRef File, StringRef BuildDirectory,
+               const SourceManager &SM, const InclusionLocations &IncLocations,
----------------
The contract around PP is really confusing: we need to be able to mutate it (to add PP hooks), we need someone to use it in the middle, we need to use its HeaderSearch structure later, and to add more confusion we have shared ownership.

Maybe instead something like:
 - this class takes an IncludeSearch& (sadly not const-correct) to use for lookups
 - it exposes a function like `std::unique_ptr<PPCallbacks> record()` which returns a callback hook which writes into `this` includeptahs object.

Then usage looks like:
```
IncludePaths Paths(SM, PreambleLocs, PP.getHeaderSearch());
PP.addPPCallbacks(Paths.record());
// do something with PP
Paths.calculate(...);
```
and AFAICT, all the lifetime issues/interactions would be communicated by the types.


================
Comment at: clangd/Headers.h:64
+  IncludePaths(StringRef File, StringRef BuildDirectory,
+               const SourceManager &SM, const InclusionLocations &IncLocations,
+               std::shared_ptr<Preprocessor> PP);
----------------
consider adding a `addExistingInclude(InclusionLocation)` instead of the `InclusionLocations` parameter here. This gives you somewhere to document what this does and what it's for (constructors are always crowded), and makes the callsites a little more self-documenting.

It also lets you take InclusionLocations out of the public interface here, which I think would be a win.


================
Comment at: clangd/Headers.h:83
+  std::string File;
+  std::string Code;
+  std::string BuildDirectory;
----------------
is this used?


================
Comment at: clangd/Headers.h:85
+  std::string BuildDirectory;
+  format::FormatStyle Style;
+
----------------
and this?


================
Comment at: clangd/Headers.h:95
+  std::shared_ptr<Preprocessor> PP;
+  std::unique_ptr<tooling::HeaderIncludes> Inserter;
+};
----------------
is this unused? this seems like it could be a natural part of this class...


================
Comment at: clangd/tool/ClangdMain.cpp:103
                    "0 means no limit."),
-    llvm::cl::init(100));
+    llvm::cl::init(1000));
 
----------------
why this change?


================
Comment at: unittests/clangd/HeadersTests.cpp:48
+    IgnoringDiagConsumer IgnoreDiags;
+    auto CI = clang::createInvocationFromCommandLine(
+        Argv,
----------------
Is there any way we can avoid creating another copy of this code?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46497