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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

LGTM

Have one question though, does it improve behavior in vscode? Since label seems to be the same, it will most definitely improve clangd's ranking but vscode ignores it anyway.



================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3165
+    }
+    if (!SeenTypedChunk && Chunk.Kind == CodeCompletionString::CK_TypedText)
+      SeenTypedChunk = true;
----------------
why not just put `SeenTypedChunk |= Chunk.Kind == CodeCompletionString::CK_TypedText`


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3172
+    // Add a space after return type.
+    if (Chunk.Kind == CodeCompletionString::CK_ResultType) {
+      assert(!SeenTypedChunk);
----------------
do we expect anything but return type in the `BeforeName` or any case where we shouldn't put a space between `BeforeName` and `NameAndSignature` ?

why not let the user add a space while concatenating these two?


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3191
+  printOverrideString(*CCS, BeforeName, NameAndSignature);
+  NameAndSignature += " override";
+
----------------
let's move this into `printOverrideString`


================
Comment at: clang/test/CodeCompletion/overrides.cpp:14
   void vfunc(bool param) override;
-  void
+  vfo
 };
----------------
nit: I suppose it should be `vfu`?(same thing for the comments below starting with `Runs completion...`


================
Comment at: clang/test/CodeCompletion/overrides.cpp:29
-//
-// Runs completion at void ^.
-// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:8 %s -o - | FileCheck -check-prefix=CHECK-CC3 %s
----------------
ilya-biryukov wrote:
> I've removed this to avoid dealing with the error return code (`vfo` is unresolved, so clang produces error there).
> Happy to bring it back if you feel it's important
it was here to prevent a regression maybe trigger it on the 13th line instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62298