[PATCH] D41281: [clangd] Index-based code completion.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.

LGTM. I still think that we should move the `SymbolIndex` out of the struct, but don't want to block this patch.



================
Comment at: clangd/CodeComplete.h:64
+
+  // Populated internally by clangd, do not set.
+  /// If `Index` is set, it is used to augment the code completion
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > ioeric wrote:
> > > ilya-biryukov wrote:
> > > > Given the comment, maybe we want to pass it as a separate parameter instead?
> > > @sammccall suggested this approach in the previous prototype patch. I also ended up preferring this approach because:
> > >  1) it doesn't require changing ClangdServer interfaces, and the dynamic index should be built by ClangdServer and thus doesn't make sense to be a parameter.
> > >  2) it enables unit tests to use customized dummy indexes.
> > >  3) we might take another (static) index in the future, and it seems easier to pass it in via the options than adding another parameter.
> > > 1. it doesn't require changing ClangdServer interfaces, and the dynamic index should be built by ClangdServer and thus doesn't make sense to be a parameter.
> > We do have it as a parameter to `codeComplete` method, the fact that it's wrapped in a struct does not make much difference.
> > `ClangdServer` should probably accept it in a constructor instead if we want to override some stuff via the dynamic index instead.
> > 
> > > 2. it enables unit tests to use customized dummy indexes.
> > unit-testing code can easily wrap any interface, this shouldn't be a problem.
> > 
> > > 3. we might take another (static) index in the future, and it seems easier to pass it in via the options than adding another parameter.
> > I'm looking at CodeCompleteOptions as a configurable user preference rather than a struct, containing all required inputs of codeComplete. I don't think SymbolIndex is in line with other fields that this struct contains.
> So yeah, this is my fault...
> I do agree it's unfortunate to have a non-user-specified param in the CodeComplete options. It's tricky - from the perspective of this module the index really is such an option, and we want to propagate it around like one. We just don't (currently) want to encourage its use as an API to clangdserver.
> 
> `codeComplete` currently has 9(!) arguments. Different people will have different reactions to this, mine is to do almost anything to avoid a 10th :-)
> 
> > unit-testing code can easily wrap any interface, this shouldn't be a problem.
> This doesn't match my experience at all. Can you suggest how to test this logic without adding parameters to at least ClangdServer::codeComplete and the codeComplete test helpers?
> 
> > ClangdServer should probably accept it in a constructor instead if we want to override some stuff via the dynamic index instead.
> As of today, the dynamic index is the only index clangd supports. There's nothing to accept in the constructor, it's entirely owned by ClangdServer.
> codeComplete currently has 9(!) arguments. Different people will have different reactions to this, mine is to do almost anything to avoid a 10th :-)
I do agree this is unfortunate and I'm not at all opposed to cleaning that up. By removing parameters that we don't need or grouping them into a struct that has saner default parameters or in a different way. Both 9 and 10 seem equally bad to me.

My point is that `SymbolIndex` should not be part of `CodeCompleteOptions` only because it makes it easier to pass it around.

> This doesn't match my experience at all. Can you suggest how to test this logic without adding parameters to at least ClangdServer::codeComplete and the codeComplete test helpers?
Exactly, I would go with the test helper.

> As of today, the dynamic index is the only index clangd supports. There's nothing to accept in the constructor, it's entirely owned by ClangdServer.
We do have implementations of the index in the tests that we pass around.


That being said, I don't want this change to be blocked by my opinion on this matter. This is a minor thing, compared to all the other changes in the patch. Just wanted to make a point that this field totally feels out of place.



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41281