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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
