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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/CodeComplete.cpp:334
+  Item.insertTextFormat = InsertTextFormat::PlainText;
+  // FIXME: sort symbols appropriately.
+  Item.sortText = "";
----------------
ioeric wrote:
> ilya-biryukov wrote:
> > NIT: FIXME(ioeric)?
> > Also, why not provide some `sortText` here? Like a qualified name. Because we're currently missing scores?
> Does `sortText` have to be non-empty? I thought the empty string would make all candidates equally scored? 
> 
> I think we want either sensible score or (for now) no score at all.
LS says that if `sortText` is empty, `label` should be used. We're probably fine here, you're right.



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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41281