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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Hope you don't mind me piling on... let me know!
Mostly readability, but i'd also like to reduce the scope w.r.t namespace completion.



================
Comment at: clangd/CodeComplete.cpp:90
 
+CompletionItemKind getKindOfSymbol(index::SymbolKind Kind) {
+  using SK = index::SymbolKind;
----------------
toCompletionItemKind?

`getKindOfSymbol` doesn't match either the input or output, and they have very similar names...

(yes, the names above are bad too - feel free to fix them!)


================
Comment at: clangd/CodeComplete.cpp:103
+    return CompletionItemKind::Enum;
+  case SK::Struct:
+  case SK::Class:
----------------
LSP has struct (not in `protocol` yet)

fine for these to be FIXME


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


================
Comment at: clangd/CodeComplete.cpp:114
+  case SK::ConversionFunction:
+    return CompletionItemKind::Function;
+  case SK::Variable:
----------------
conversionfunction might be rather operator, not sure


================
Comment at: clangd/CodeComplete.cpp:121
+  case SK::EnumConstant:
+    return CompletionItemKind::Value;
+  case SK::InstanceMethod:
----------------
LSP has enum constant, not in `protocol.h` yet


================
Comment at: clangd/CodeComplete.cpp:131
+  case SK::Constructor:
+  case SK::Destructor:
+    return CompletionItemKind::Constructor;
----------------
destructor seems more like an operator or method


================
Comment at: clangd/CodeComplete.cpp:280
 
+/// \brief Information about the scope specifier in the qualfiied-id code
+/// completion (e.g. "ns::ab?").
----------------
qualified


================
Comment at: clangd/CodeComplete.cpp:282
+/// completion (e.g. "ns::ab?").
+struct ScopeSpecifierInfo {
+  static ScopeSpecifierInfo create(Sema &S, const CXXScopeSpec &SS) {
----------------
nit: 'info' is just noise. Specifier is vague too - consider calling this SpecifiedScope, putting the emphasis on the user interaction.


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


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


================
Comment at: clangd/CodeComplete.cpp:310
+  // "ns::ab?", the range will be "ns".
+  unsigned int SpecifierBeginOffset;
+  unsigned int SpecifierEndOffset;
----------------
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 }`


================
Comment at: clangd/CodeComplete.cpp:322
+
+CompletionItem symbolToCompletionItem(const Symbol &Sym, llvm::StringRef Code,
+                                      const ScopeSpecifierInfo &SSInfo) {
----------------
nit: `symbol` is impossibly vague here - since we're always talking about symbols and it's hard to know you mean index::Symbol.

I'd call this `indexCompletionItem`, but just `toCompletionItem` seems fine too


================
Comment at: clangd/CodeComplete.cpp:325
+  CompletionItem Item;
+  bool FullyQualified =
+      llvm::StringRef(SSInfo.WrittenSpecifier).startswith("::");
----------------
as discussed offline: fuzzy matching and rewriting qualifiers is a can of worms.
I'd suggest we don't open it yet: just complete the symbols from exactly the typed scope.
Next we can work on making global completion work (empty qualifier can complete any namespace's symbols).
Finally we'll see if we need to tackle fuzzy-matching the parent namespace.

Among other things, this means that the first step doesn't have to rewrite qualifiers at all, the second only needs to insert them, and we defer thorny cases until the third.



================
Comment at: clangd/CodeComplete.cpp:347
+
+void qualifiedIdCompletionWithIndex(const Context &Ctx,
+                                    const SymbolIndex &Index,
----------------
we're a bit lax about the "method name should start with a verb" rule, but this one I actually found hard to parse.

`completeWithIndex`? (since there's no unqualified case yet, and there's no need to guess about how the code would be structured at this point)


================
Comment at: clangd/CodeComplete.cpp:362
+  if (!Filter.empty())
+    Req.Query += Filter;
+
----------------
suggest we do this in a structured way instead:
`Index::fuzzyFind` query has a vector<string> of namespaces to look up in


================
Comment at: clangd/CodeComplete.cpp:373
+
+/// brief Collects information about an invocation of sema code completion.
+struct SemaCompletionInfo {
----------------
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 :-(


================
Comment at: clangd/CodeComplete.cpp:375
+struct SemaCompletionInfo {
+  /// Code completion filter.
+  std::string Filter;
----------------
the partial identifier that is being completed, without qualifiers


================
Comment at: clangd/CodeComplete.cpp:380
+  llvm::Optional<ScopeSpecifierInfo> SSInfo;
+  // FIXME: add more information for other completion cases that we care about.
+  // For example, non-qualified id completion.
----------------
It's not clear what's to fix here - isn't non-qualified ID completion already represented by {Filter, None}?
(Feel free just to remove the comment if it's not clear what's to be done)


================
Comment at: clangd/CodeComplete.cpp:397
                                   unsigned NumResults) override final {
-    StringRef Filter = S.getPreprocessor().getCodeCompletionFilter();
+    if (llvm::Optional<const CXXScopeSpec *> SS =
+            Context.getCXXScopeSpecifier())
----------------
nit: auto seems a lot easier to read here?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41281