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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clangd/CodeComplete.cpp:111
+  case SK::Using:
+    return CompletionItemKind::Reference;
+  case SK::Function:
----------------
ioeric wrote:
> sammccall wrote:
> > this seems wrong? I would have thought references are similar to c++ references?
> > can't find anything in LSP either way.
> > 
> > TypeAlias seems like it could default to class, for Using is complicated (would need to look at the subtype) - maybe just unknown?
> TypeAlias could be more than class I think? And they are references (to types) to some extend. Anyhow, this is copied from the conversion function above. I'll leave a `FIXME` for now.
Yeah, this is the problem I was alluding to before - LSP encourages us to be very specific in completion kinds - but we can't always be. Class seems like the best guess to me, but FIXME is also fine for now.


================
Comment at: clangd/CodeComplete.cpp:283
+struct ScopeSpecifierInfo {
+  static ScopeSpecifierInfo create(Sema &S, const CXXScopeSpec &SS) {
+    ScopeSpecifierInfo Info;
----------------
ioeric wrote:
> sammccall wrote:
> > sammccall wrote:
> > > There's a lot of sema/ast details here, wedged in the middle of the code that deals with candidates and scoring. Can you forward declare this, and move this implementation and the other details somewhere lower?
> > "create" doesn't give any more semantics than a constructor would.
> > 
> > Maybe `extractCompletionScope`? (This could also be a free function)
> This is moved to a standalone function `extractCompletionScope`.
Thanks for extracting the function. These index-related details are still out of place - they split the two chunks of code that mostly do sema-based completion.
Can you move `indexCompletionItem`, `completeWithIndex`, and the implementation of `extractCompletionScope` below? I'd suggest immediately above `codeComplete()`.


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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41281