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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thanks for the review!



================
Comment at: clangd/CodeComplete.cpp:847
+  if (Opts.Index && SCInfo.SSInfo) {
+    // FIXME: log warning with logger if sema code completion have collected
+    // results.
----------------
ilya-biryukov wrote:
> NIT: FIXME(ioeric)? 
> Also, what's missing to resolve this FIXME in this commit?
Didn't realize logger is ready to use. Switched to use logger.


================
Comment at: clangd/CodeComplete.cpp:111
+  case SK::Using:
+    return CompletionItemKind::Reference;
+  case SK::Function:
----------------
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.


================
Comment at: clangd/CodeComplete.cpp:114
+  case SK::ConversionFunction:
+    return CompletionItemKind::Function;
+  case SK::Variable:
----------------
sammccall wrote:
> conversionfunction might be rather operator, not sure
Operator is not in the protocol either. I leave a FIXME and will clean it up in followup.


================
Comment at: clangd/CodeComplete.cpp:283
+struct ScopeSpecifierInfo {
+  static ScopeSpecifierInfo create(Sema &S, const CXXScopeSpec &SS) {
+    ScopeSpecifierInfo Info;
----------------
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`.


================
Comment at: clangd/CodeComplete.cpp:310
+  // "ns::ab?", the range will be "ns".
+  unsigned int SpecifierBeginOffset;
+  unsigned int SpecifierEndOffset;
----------------
sammccall wrote:
> The written/resolved distinction is tangled up with the various fields relating to the written form.
> 
> Could you pull out a struct here like:
> `struct SourceRange {size_t Begin; size_t End; std::string Text}`
> 
> (This might be a reasonable candidate for `SourceCode.h)
> 
> Then this struct can become `struct ScopeSpecifier { SourceRange Written, std::string Resolved }`
Got rid of ranges as we don't need any them in this revision.


================
Comment at: clangd/CodeComplete.cpp:334
+  Item.insertTextFormat = InsertTextFormat::PlainText;
+  // FIXME: sort symbols appropriately.
+  Item.sortText = "";
----------------
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.


================
Comment at: clangd/CodeComplete.cpp:373
+
+/// brief Collects information about an invocation of sema code completion.
+struct SemaCompletionInfo {
----------------
sammccall wrote:
> Please look for names that will tell the reader what the role of this thing is, and only add comments that provide extra information.
> 
> `SemaCompletionInfo` accurately describes most of the types in this file!
> 
> Best i can come up with is `NameToComplete` or `LeadingText` - not great :-(
`NameToComplete` sounds reasonable.


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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41281