[PATCH] D62298: [CodeComplete] Filter override completions by function name

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu May 23 06:39:36 PDT 2019


ilya-biryukov added inline comments.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3166
+    // Add a space after return type.
+    if (Chunk.Kind == CodeCompletionString::CK_ResultType)
+      Result.AddChunk(CodeCompletionString::CK_HorizontalSpace);
----------------
kadircet wrote:
> I believe it is sensible to make resulttype a typed text as well, because people might start typing the function signature instead of name if they are not familiar with the feature
Yeah, I can see why that looks appealing (return type is the prefix of completion item, so it feels most natural to type it), but that's exactly what renders the fuzzy match scores unreliable. Other items are matched by name, so to give the match scores a meaningful interpretation across different completion items we should match by name whenever we can.

Reasons why it felt like matching names instead of return types is better:
1) some return types are incredibly common, e.g. `bool` and `void`. If they are part of the typed text, the users can either (1) type `void` or `bool` and get too many results (tried that in `PPCallbacks` descendants) or (2) type the function name and get the override completions will be penalized because the name of the function is too far away in the text we try to match.
2) it feels like remembering names of functions you want to override is more common than remembering return types of functions you to override.



================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3182
+  Result.AddChunk(CodeCompletionString::CK_HorizontalSpace);
+  Result.AddTextChunk("override");
   return Result.TakeString();
----------------
kadircet wrote:
> I believe we should  make `override` a typed text as well.
Agree that would be nice and I actually tried it. But it didn't work: we can only have one typed text chunk, e.g. `clangd` will break if there are more than one and maybe other tools have this assumption too.

Would be nice to allow this (by fixing the code to not have this assumption, I guess), but I'd avoid doing this in this patch. Should I add a FIXME?


================
Comment at: clang/test/CodeCompletion/overrides.cpp:26
 // CHECK-CC2-NOT: COMPLETION: Pattern : int ttt(bool param, int x = 3) const override{{$}}
+// CHECK-CC2: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // CHECK-CC2-NOT: COMPLETION: Pattern : void vfunc(bool param) override{{$}}
----------------
kadircet wrote:
> why move this ?
Order matters for `-NOT` matches and I believe it was wrong before (given checks marked with `CHECK-CC1`)

E.g. 
```
// CHECK: a
// CHECK-NOT: b
```
will match without error against 
```
b
a
```
and give an error on
```
b
a
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62298/new/

https://reviews.llvm.org/D62298





More information about the cfe-commits mailing list