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

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE361623: [CodeComplete] Filter override completions by 
function name (authored by ibiryukov, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D62298?vs=201168=201178#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D62298

Files:
  clangd/unittests/CodeCompleteTests.cpp


Index: clangd/unittests/CodeCompleteTests.cpp
===
--- clangd/unittests/CodeCompleteTests.cpp
+++ clangd/unittests/CodeCompleteTests.cpp
@@ -49,6 +49,9 @@
 
 // GMock helpers for matching completion items.
 MATCHER_P(Named, Name, "") { return arg.Name == Name; }
+MATCHER_P(NameStartsWith, Prefix, "") {
+  return llvm::StringRef(arg.Name).startswith(Prefix);
+}
 MATCHER_P(Scope, S, "") { return arg.Scope == S; }
 MATCHER_P(Qualifier, Q, "") { return arg.RequiredQualifier == Q; }
 MATCHER_P(Labeled, Label, "") {
@@ -1946,10 +1949,13 @@
   };
   )cpp");
   const auto Results = completions(Text);
-  EXPECT_THAT(Results.Completions,
-  AllOf(Contains(Labeled("void vfunc(bool param, int p) 
override")),
-Contains(Labeled("void ttt(bool param) const override")),
-Not(Contains(Labeled("void vfunc(bool param) 
override");
+  EXPECT_THAT(
+  Results.Completions,
+  AllOf(Contains(AllOf(Labeled("void vfunc(bool param, int p) override"),
+   NameStartsWith("vfunc"))),
+Contains(AllOf(Labeled("void ttt(bool param) const override"),
+   NameStartsWith("ttt"))),
+Not(Contains(Labeled("void vfunc(bool param) override");
 }
 
 TEST(CompletionTest, OverridesNonIdentName) {


Index: clangd/unittests/CodeCompleteTests.cpp
===
--- clangd/unittests/CodeCompleteTests.cpp
+++ clangd/unittests/CodeCompleteTests.cpp
@@ -49,6 +49,9 @@
 
 // GMock helpers for matching completion items.
 MATCHER_P(Named, Name, "") { return arg.Name == Name; }
+MATCHER_P(NameStartsWith, Prefix, "") {
+  return llvm::StringRef(arg.Name).startswith(Prefix);
+}
 MATCHER_P(Scope, S, "") { return arg.Scope == S; }
 MATCHER_P(Qualifier, Q, "") { return arg.RequiredQualifier == Q; }
 MATCHER_P(Labeled, Label, "") {
@@ -1946,10 +1949,13 @@
   };
   )cpp");
   const auto Results = completions(Text);
-  EXPECT_THAT(Results.Completions,
-  AllOf(Contains(Labeled("void vfunc(bool param, int p) override")),
-Contains(Labeled("void ttt(bool param) const override")),
-Not(Contains(Labeled("void vfunc(bool param) override");
+  EXPECT_THAT(
+  Results.Completions,
+  AllOf(Contains(AllOf(Labeled("void vfunc(bool param, int p) override"),
+   NameStartsWith("vfunc"))),
+Contains(AllOf(Labeled("void ttt(bool param) const override"),
+   NameStartsWith("ttt"))),
+Not(Contains(Labeled("void vfunc(bool param) override");
 }
 
 TEST(CompletionTest, OverridesNonIdentName) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 201168.
ilya-biryukov added a comment.

- Update a comment in the test
- Reformat the code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62298

Files:
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/overrides.cpp

Index: clang/test/CodeCompletion/overrides.cpp
===
--- clang/test/CodeCompletion/overrides.cpp
+++ clang/test/CodeCompletion/overrides.cpp
@@ -11,23 +11,23 @@
 class C : public B {
  public:
   void vfunc(bool param) override;
-  void
+  vf
 };
 
-// Runs completion at ^void.
+// Runs completion at ^vf
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:3 %s -o - | FileCheck -check-prefix=CHECK-CC1 %s
 // CHECK-CC1: COMPLETION: Pattern : int ttt(bool param, int x = 3) const override{{$}}
 // CHECK-CC1: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // CHECK-CC1-NOT: COMPLETION: Pattern : void vfunc(bool param) override{{$}}
 //
-// Runs completion at vo^id.
+// Runs completion at vf^
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:5 %s -o - | FileCheck -check-prefix=CHECK-CC2 %s
-// CHECK-CC2: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // 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{{$}}
 //
-// Runs completion at void ^.
-// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:8 %s -o - | FileCheck -check-prefix=CHECK-CC3 %s
+// Runs completion at void ^ on line 13.
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:13:8 %s -o - | FileCheck -check-prefix=CHECK-CC3 %s
 // CHECK-CC3-NOT: COMPLETION: Pattern : int ttt(bool param, int x = 3) const override{{$}}
 // CHECK-CC3-NOT: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // CHECK-CC3-NOT: COMPLETION: Pattern : void vfunc(bool param) override{{$}}
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -37,6 +37,7 @@
 #include "llvm/Support/Path.h"
 #include 
 #include 
+#include 
 #include 
 
 using namespace clang;
@@ -1828,19 +1829,6 @@
   Results.AddResult(CodeCompletionResult(Builder.TakeString()));
 }
 
-static void printOverrideString(llvm::raw_ostream ,
-CodeCompletionString *CCS) {
-  for (const auto  : *CCS) {
-if (C.Kind == CodeCompletionString::CK_Optional)
-  printOverrideString(OS, C.Optional);
-else
-  OS << C.Text;
-// Add a space after return type.
-if (C.Kind == CodeCompletionString::CK_ResultType)
-  OS << ' ';
-  }
-}
-
 static void AddOverrideResults(ResultBuilder ,
const CodeCompletionContext ,
CodeCompletionBuilder ) {
@@ -3162,19 +3150,43 @@
   PP, Ctx, Result, IncludeBriefComments, CCContext, Policy);
 }
 
+static void printOverrideString(const CodeCompletionString ,
+std::string ,
+std::string ) {
+  bool SeenTypedChunk = false;
+  for (auto  : CCS) {
+if (Chunk.Kind == CodeCompletionString::CK_Optional) {
+  assert(SeenTypedChunk && "optional parameter before name");
+  // Note that we put all chunks inside into NameAndSignature.
+  printOverrideString(*Chunk.Optional, NameAndSignature, NameAndSignature);
+  continue;
+}
+SeenTypedChunk |= Chunk.Kind == CodeCompletionString::CK_TypedText;
+if (SeenTypedChunk)
+  NameAndSignature += Chunk.Text;
+else
+  BeforeName += Chunk.Text;
+  }
+}
+
 CodeCompletionString *
 CodeCompletionResult::createCodeCompletionStringForOverride(
 Preprocessor , ASTContext , CodeCompletionBuilder ,
 bool IncludeBriefComments, const CodeCompletionContext ,
 PrintingPolicy ) {
-  std::string OverrideSignature;
-  llvm::raw_string_ostream OS(OverrideSignature);
   auto *CCS = createCodeCompletionStringForDecl(PP, Ctx, Result,
 /*IncludeBriefComments=*/false,
 CCContext, Policy);
-  printOverrideString(OS, CCS);
-  OS << " override";
-  Result.AddTypedTextChunk(Result.getAllocator().CopyString(OS.str()));
+  std::string BeforeName;
+  std::string NameAndSignature;
+  // For overrides all chunks go into the result, none are informative.
+  printOverrideString(*CCS, BeforeName, NameAndSignature);
+  NameAndSignature += " override";
+
+  Result.AddTextChunk(Result.getAllocator().CopyString(BeforeName));
+  Result.AddChunk(CodeCompletionString::CK_HorizontalSpace);
+  

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 201167.
ilya-biryukov added a comment.

- Remove redundant test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62298

Files:
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/overrides.cpp

Index: clang/test/CodeCompletion/overrides.cpp
===
--- clang/test/CodeCompletion/overrides.cpp
+++ clang/test/CodeCompletion/overrides.cpp
@@ -11,23 +11,23 @@
 class C : public B {
  public:
   void vfunc(bool param) override;
-  void
+  vf
 };
 
-// Runs completion at ^void.
+// Runs completion at ^vfo
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:3 %s -o - | FileCheck -check-prefix=CHECK-CC1 %s
 // CHECK-CC1: COMPLETION: Pattern : int ttt(bool param, int x = 3) const override{{$}}
 // CHECK-CC1: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // CHECK-CC1-NOT: COMPLETION: Pattern : void vfunc(bool param) override{{$}}
 //
-// Runs completion at vo^id.
+// Runs completion at vf^o
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:5 %s -o - | FileCheck -check-prefix=CHECK-CC2 %s
-// CHECK-CC2: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // 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{{$}}
 //
-// Runs completion at void ^.
-// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:8 %s -o - | FileCheck -check-prefix=CHECK-CC3 %s
+// Runs completion at void ^ on line 13.
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:13:8 %s -o - | FileCheck -check-prefix=CHECK-CC3 %s
 // CHECK-CC3-NOT: COMPLETION: Pattern : int ttt(bool param, int x = 3) const override{{$}}
 // CHECK-CC3-NOT: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // CHECK-CC3-NOT: COMPLETION: Pattern : void vfunc(bool param) override{{$}}
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -37,6 +37,7 @@
 #include "llvm/Support/Path.h"
 #include 
 #include 
+#include 
 #include 
 
 using namespace clang;
@@ -1828,19 +1829,6 @@
   Results.AddResult(CodeCompletionResult(Builder.TakeString()));
 }
 
-static void printOverrideString(llvm::raw_ostream ,
-CodeCompletionString *CCS) {
-  for (const auto  : *CCS) {
-if (C.Kind == CodeCompletionString::CK_Optional)
-  printOverrideString(OS, C.Optional);
-else
-  OS << C.Text;
-// Add a space after return type.
-if (C.Kind == CodeCompletionString::CK_ResultType)
-  OS << ' ';
-  }
-}
-
 static void AddOverrideResults(ResultBuilder ,
const CodeCompletionContext ,
CodeCompletionBuilder ) {
@@ -3162,19 +3150,44 @@
   PP, Ctx, Result, IncludeBriefComments, CCContext, Policy);
 }
 
+static void printOverrideString(const CodeCompletionString ,
+   std::string ,
+   std::string ) {
+  bool SeenTypedChunk = false;
+  for (auto  : CCS) {
+if (Chunk.Kind == CodeCompletionString::CK_Optional) {
+  assert(SeenTypedChunk && "optional parameter before name");
+  // Note that we put all chunks inside into NameAndSignature.
+  printOverrideString(*Chunk.Optional, NameAndSignature,
+ NameAndSignature);
+  continue;
+}
+SeenTypedChunk |= Chunk.Kind == CodeCompletionString::CK_TypedText;
+if (SeenTypedChunk)
+  NameAndSignature += Chunk.Text;
+else
+  BeforeName += Chunk.Text;
+  }
+}
+
 CodeCompletionString *
 CodeCompletionResult::createCodeCompletionStringForOverride(
 Preprocessor , ASTContext , CodeCompletionBuilder ,
 bool IncludeBriefComments, const CodeCompletionContext ,
 PrintingPolicy ) {
-  std::string OverrideSignature;
-  llvm::raw_string_ostream OS(OverrideSignature);
   auto *CCS = createCodeCompletionStringForDecl(PP, Ctx, Result,
 /*IncludeBriefComments=*/false,
 CCContext, Policy);
-  printOverrideString(OS, CCS);
-  OS << " override";
-  Result.AddTypedTextChunk(Result.getAllocator().CopyString(OS.str()));
+  std::string BeforeName;
+  std::string NameAndSignature;
+  // For overrides all chunks go into the result, none are informative.
+  printOverrideString(*CCS, BeforeName, NameAndSignature);
+  NameAndSignature += " override";
+
+  Result.AddTextChunk(Result.getAllocator().CopyString(BeforeName));
+  Result.AddChunk(CodeCompletionString::CK_HorizontalSpace);

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 201164.
ilya-biryukov added a comment.

- Do not add an extra 'override' on optional chunks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62298

Files:
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/overrides.cpp

Index: clang/test/CodeCompletion/overrides.cpp
===
--- clang/test/CodeCompletion/overrides.cpp
+++ clang/test/CodeCompletion/overrides.cpp
@@ -11,23 +11,23 @@
 class C : public B {
  public:
   void vfunc(bool param) override;
-  void
+  vfo
 };
 
-// Runs completion at ^void.
+// Runs completion at ^vfo
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:3 %s -o - | FileCheck -check-prefix=CHECK-CC1 %s
 // CHECK-CC1: COMPLETION: Pattern : int ttt(bool param, int x = 3) const override{{$}}
 // CHECK-CC1: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // CHECK-CC1-NOT: COMPLETION: Pattern : void vfunc(bool param) override{{$}}
 //
-// Runs completion at vo^id.
+// Runs completion at vf^o
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:5 %s -o - | FileCheck -check-prefix=CHECK-CC2 %s
-// CHECK-CC2: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // 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{{$}}
 //
-// Runs completion at void ^.
-// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:8 %s -o - | FileCheck -check-prefix=CHECK-CC3 %s
+// Runs completion at void ^ on line 13.
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:13:8 %s -o - | FileCheck -check-prefix=CHECK-CC3 %s
 // CHECK-CC3-NOT: COMPLETION: Pattern : int ttt(bool param, int x = 3) const override{{$}}
 // CHECK-CC3-NOT: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // CHECK-CC3-NOT: COMPLETION: Pattern : void vfunc(bool param) override{{$}}
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -37,6 +37,7 @@
 #include "llvm/Support/Path.h"
 #include 
 #include 
+#include 
 #include 
 
 using namespace clang;
@@ -1828,19 +1829,6 @@
   Results.AddResult(CodeCompletionResult(Builder.TakeString()));
 }
 
-static void printOverrideString(llvm::raw_ostream ,
-CodeCompletionString *CCS) {
-  for (const auto  : *CCS) {
-if (C.Kind == CodeCompletionString::CK_Optional)
-  printOverrideString(OS, C.Optional);
-else
-  OS << C.Text;
-// Add a space after return type.
-if (C.Kind == CodeCompletionString::CK_ResultType)
-  OS << ' ';
-  }
-}
-
 static void AddOverrideResults(ResultBuilder ,
const CodeCompletionContext ,
CodeCompletionBuilder ) {
@@ -3162,19 +3150,44 @@
   PP, Ctx, Result, IncludeBriefComments, CCContext, Policy);
 }
 
+static void printOverrideString(const CodeCompletionString ,
+   std::string ,
+   std::string ) {
+  bool SeenTypedChunk = false;
+  for (auto  : CCS) {
+if (Chunk.Kind == CodeCompletionString::CK_Optional) {
+  assert(SeenTypedChunk && "optional parameter before name");
+  // Note that we put all chunks inside into NameAndSignature.
+  printOverrideString(*Chunk.Optional, NameAndSignature,
+ NameAndSignature);
+  continue;
+}
+SeenTypedChunk |= Chunk.Kind == CodeCompletionString::CK_TypedText;
+if (SeenTypedChunk)
+  NameAndSignature += Chunk.Text;
+else
+  BeforeName += Chunk.Text;
+  }
+}
+
 CodeCompletionString *
 CodeCompletionResult::createCodeCompletionStringForOverride(
 Preprocessor , ASTContext , CodeCompletionBuilder ,
 bool IncludeBriefComments, const CodeCompletionContext ,
 PrintingPolicy ) {
-  std::string OverrideSignature;
-  llvm::raw_string_ostream OS(OverrideSignature);
   auto *CCS = createCodeCompletionStringForDecl(PP, Ctx, Result,
 /*IncludeBriefComments=*/false,
 CCContext, Policy);
-  printOverrideString(OS, CCS);
-  OS << " override";
-  Result.AddTypedTextChunk(Result.getAllocator().CopyString(OS.str()));
+  std::string BeforeName;
+  std::string NameAndSignature;
+  // For overrides all chunks go into the result, none are informative.
+  printOverrideString(*CCS, BeforeName, NameAndSignature);
+  NameAndSignature += " override";
+
+  Result.AddTextChunk(Result.getAllocator().CopyString(BeforeName));
+  

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3172
+// Add a space after return type.
+if (Chunk.Kind == CodeCompletionString::CK_ResultType) {
+  assert(!SeenTypedChunk);

kadircet wrote:
> ilya-biryukov wrote:
> > kadircet wrote:
> > > 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?
> > Maybe annotations? But not sure where they go (also not sure we should 
> > print them here).
> > I could move adding the space to the point where we concatenate 
> > `BeforeName` and `NameAndSignature` if that would make it clearer.
> I usually find it annoying when some helper function returns strings with 
> trailing whitespaces, which is not useful in the general case. (Faced those a 
> lot during `HoverInfo` patch.) I believe it would make the function more 
> usable to others, so let's make it part of concat logic.
Agree, it's cleaner that way.


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

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 201161.
ilya-biryukov marked 4 inline comments as done.
ilya-biryukov added a comment.

- Add whitespace outside printOverrideString


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62298

Files:
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/overrides.cpp

Index: clang/test/CodeCompletion/overrides.cpp
===
--- clang/test/CodeCompletion/overrides.cpp
+++ clang/test/CodeCompletion/overrides.cpp
@@ -11,23 +11,23 @@
 class C : public B {
  public:
   void vfunc(bool param) override;
-  void
+  vfo
 };
 
-// Runs completion at ^void.
+// Runs completion at ^vfo
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:3 %s -o - | FileCheck -check-prefix=CHECK-CC1 %s
 // CHECK-CC1: COMPLETION: Pattern : int ttt(bool param, int x = 3) const override{{$}}
 // CHECK-CC1: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // CHECK-CC1-NOT: COMPLETION: Pattern : void vfunc(bool param) override{{$}}
 //
-// Runs completion at vo^id.
+// Runs completion at vf^o
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:5 %s -o - | FileCheck -check-prefix=CHECK-CC2 %s
-// CHECK-CC2: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // 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{{$}}
 //
-// Runs completion at void ^.
-// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:8 %s -o - | FileCheck -check-prefix=CHECK-CC3 %s
+// Runs completion at void ^ on line 13.
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:13:8 %s -o - | FileCheck -check-prefix=CHECK-CC3 %s
 // CHECK-CC3-NOT: COMPLETION: Pattern : int ttt(bool param, int x = 3) const override{{$}}
 // CHECK-CC3-NOT: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // CHECK-CC3-NOT: COMPLETION: Pattern : void vfunc(bool param) override{{$}}
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -37,6 +37,7 @@
 #include "llvm/Support/Path.h"
 #include 
 #include 
+#include 
 #include 
 
 using namespace clang;
@@ -1828,19 +1829,6 @@
   Results.AddResult(CodeCompletionResult(Builder.TakeString()));
 }
 
-static void printOverrideString(llvm::raw_ostream ,
-CodeCompletionString *CCS) {
-  for (const auto  : *CCS) {
-if (C.Kind == CodeCompletionString::CK_Optional)
-  printOverrideString(OS, C.Optional);
-else
-  OS << C.Text;
-// Add a space after return type.
-if (C.Kind == CodeCompletionString::CK_ResultType)
-  OS << ' ';
-  }
-}
-
 static void AddOverrideResults(ResultBuilder ,
const CodeCompletionContext ,
CodeCompletionBuilder ) {
@@ -3162,19 +3150,44 @@
   PP, Ctx, Result, IncludeBriefComments, CCContext, Policy);
 }
 
+static void printOverrideString(const CodeCompletionString ,
+   std::string ,
+   std::string ) {
+  bool SeenTypedChunk = false;
+  for (auto  : CCS) {
+if (Chunk.Kind == CodeCompletionString::CK_Optional) {
+  assert(SeenTypedChunk && "optional parameter before name");
+  // Note that we put all chunks inside into NameAndSignature.
+  printOverrideString(*Chunk.Optional, NameAndSignature,
+ NameAndSignature);
+  continue;
+}
+SeenTypedChunk |= Chunk.Kind == CodeCompletionString::CK_TypedText;
+if (SeenTypedChunk)
+  NameAndSignature += Chunk.Text;
+else
+  BeforeName += Chunk.Text;
+  }
+  NameAndSignature += " override";
+}
+
 CodeCompletionString *
 CodeCompletionResult::createCodeCompletionStringForOverride(
 Preprocessor , ASTContext , CodeCompletionBuilder ,
 bool IncludeBriefComments, const CodeCompletionContext ,
 PrintingPolicy ) {
-  std::string OverrideSignature;
-  llvm::raw_string_ostream OS(OverrideSignature);
   auto *CCS = createCodeCompletionStringForDecl(PP, Ctx, Result,
 /*IncludeBriefComments=*/false,
 CCContext, Policy);
-  printOverrideString(OS, CCS);
-  OS << " override";
-  Result.AddTypedTextChunk(Result.getAllocator().CopyString(OS.str()));
+  std::string BeforeName;
+  std::string NameAndSignature;
+  // For overrides all chunks go into the result, none are informative.
+  printOverrideString(*CCS, BeforeName, NameAndSignature);
+
+  

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 201157.
ilya-biryukov marked 4 inline comments as done.
ilya-biryukov added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62298

Files:
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/overrides.cpp

Index: clang/test/CodeCompletion/overrides.cpp
===
--- clang/test/CodeCompletion/overrides.cpp
+++ clang/test/CodeCompletion/overrides.cpp
@@ -11,23 +11,23 @@
 class C : public B {
  public:
   void vfunc(bool param) override;
-  void
+  vfo
 };
 
-// Runs completion at ^void.
+// Runs completion at ^vfo
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:3 %s -o - | FileCheck -check-prefix=CHECK-CC1 %s
 // CHECK-CC1: COMPLETION: Pattern : int ttt(bool param, int x = 3) const override{{$}}
 // CHECK-CC1: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // CHECK-CC1-NOT: COMPLETION: Pattern : void vfunc(bool param) override{{$}}
 //
-// Runs completion at vo^id.
+// Runs completion at vf^o
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:5 %s -o - | FileCheck -check-prefix=CHECK-CC2 %s
-// CHECK-CC2: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // 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{{$}}
 //
-// Runs completion at void ^.
-// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:8 %s -o - | FileCheck -check-prefix=CHECK-CC3 %s
+// Runs completion at void ^ on line 13.
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:13:8 %s -o - | FileCheck -check-prefix=CHECK-CC3 %s
 // CHECK-CC3-NOT: COMPLETION: Pattern : int ttt(bool param, int x = 3) const override{{$}}
 // CHECK-CC3-NOT: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // CHECK-CC3-NOT: COMPLETION: Pattern : void vfunc(bool param) override{{$}}
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -37,6 +37,7 @@
 #include "llvm/Support/Path.h"
 #include 
 #include 
+#include 
 #include 
 
 using namespace clang;
@@ -1828,19 +1829,6 @@
   Results.AddResult(CodeCompletionResult(Builder.TakeString()));
 }
 
-static void printOverrideString(llvm::raw_ostream ,
-CodeCompletionString *CCS) {
-  for (const auto  : *CCS) {
-if (C.Kind == CodeCompletionString::CK_Optional)
-  printOverrideString(OS, C.Optional);
-else
-  OS << C.Text;
-// Add a space after return type.
-if (C.Kind == CodeCompletionString::CK_ResultType)
-  OS << ' ';
-  }
-}
-
 static void AddOverrideResults(ResultBuilder ,
const CodeCompletionContext ,
CodeCompletionBuilder ) {
@@ -3162,19 +3150,48 @@
   PP, Ctx, Result, IncludeBriefComments, CCContext, Policy);
 }
 
+static void printOverrideString(const CodeCompletionString ,
+   std::string ,
+   std::string ) {
+  bool SeenTypedChunk = false;
+  for (auto  : CCS) {
+if (Chunk.Kind == CodeCompletionString::CK_Optional) {
+  assert(SeenTypedChunk && "optional parameter before name");
+  // Note that we put all chunks inside into NameAndSignature.
+  printOverrideString(*Chunk.Optional, NameAndSignature,
+ NameAndSignature);
+  continue;
+}
+SeenTypedChunk |= Chunk.Kind == CodeCompletionString::CK_TypedText;
+if (SeenTypedChunk)
+  NameAndSignature += Chunk.Text;
+else
+  BeforeName += Chunk.Text;
+// Add a space after return type.
+if (Chunk.Kind == CodeCompletionString::CK_ResultType) {
+  assert(!SeenTypedChunk);
+  BeforeName += " ";
+}
+  }
+  NameAndSignature += " override";
+}
+
 CodeCompletionString *
 CodeCompletionResult::createCodeCompletionStringForOverride(
 Preprocessor , ASTContext , CodeCompletionBuilder ,
 bool IncludeBriefComments, const CodeCompletionContext ,
 PrintingPolicy ) {
-  std::string OverrideSignature;
-  llvm::raw_string_ostream OS(OverrideSignature);
   auto *CCS = createCodeCompletionStringForDecl(PP, Ctx, Result,
 /*IncludeBriefComments=*/false,
 CCContext, Policy);
-  printOverrideString(OS, CCS);
-  OS << " override";
-  Result.AddTypedTextChunk(Result.getAllocator().CopyString(OS.str()));
+  std::string BeforeName;
+  std::string NameAndSignature;
+  // For overrides all chunks go into the result, none 

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang/test/CodeCompletion/overrides.cpp:14
   void vfunc(bool param) override;
-  void
+  vfo
 };

kadircet wrote:
> nit: I suppose it should be `vfu`?(same thing for the comments below starting 
> with `Runs completion...`
No, the idea was to test the `vfunc` item prefix-matching on `vfo` but still 
matches on `vf`


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

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3172
+// Add a space after return type.
+if (Chunk.Kind == CodeCompletionString::CK_ResultType) {
+  assert(!SeenTypedChunk);

ilya-biryukov wrote:
> kadircet wrote:
> > 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?
> Maybe annotations? But not sure where they go (also not sure we should print 
> them here).
> I could move adding the space to the point where we concatenate `BeforeName` 
> and `NameAndSignature` if that would make it clearer.
I usually find it annoying when some helper function returns strings with 
trailing whitespaces, which is not useful in the general case. (Faced those a 
lot during `HoverInfo` patch.) I believe it would make the function more usable 
to others, so let's make it part of concat logic.


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

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3172
+// Add a space after return type.
+if (Chunk.Kind == CodeCompletionString::CK_ResultType) {
+  assert(!SeenTypedChunk);

kadircet wrote:
> 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?
Maybe annotations? But not sure where they go (also not sure we should print 
them here).
I could move adding the space to the point where we concatenate `BeforeName` 
and `NameAndSignature` if that would make it clearer.


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

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D62298#1515399 , @kadircet wrote:

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


It does, VSCode relies on `filterText` for ranking and since it now starts with 
the function name the ranking is much better.

Will fix the rest of the comments and land this.


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

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

After landing this, will try to add new presentation options for completion 
items here.


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

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

I think that's a good step forward, although not yet ideal. The typed chunk now 
contains everything starting function name and ending with `override`, so one 
gets both nice prefix match scores when typing a function and possibility to 
say `override` to get all completion items that do overrides.




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

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


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

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 201000.
ilya-biryukov added a comment.
Herald added subscribers: arphaman, jkorous.

- Make first letter of the helper function lowercase
- New model: everything before name is a text chunk, everything after it is 
typed chunk
- Test the filter text produced by clangd


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62298

Files:
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/overrides.cpp

Index: clang/test/CodeCompletion/overrides.cpp
===
--- clang/test/CodeCompletion/overrides.cpp
+++ clang/test/CodeCompletion/overrides.cpp
@@ -11,23 +11,17 @@
 class C : public B {
  public:
   void vfunc(bool param) override;
-  void
+  vfo
 };
 
-// Runs completion at ^void.
+// Runs completion at ^vfo
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:3 %s -o - | FileCheck -check-prefix=CHECK-CC1 %s
 // CHECK-CC1: COMPLETION: Pattern : int ttt(bool param, int x = 3) const override{{$}}
 // CHECK-CC1: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // CHECK-CC1-NOT: COMPLETION: Pattern : void vfunc(bool param) override{{$}}
 //
-// Runs completion at vo^id.
+// Runs completion at vf^o
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:5 %s -o - | FileCheck -check-prefix=CHECK-CC2 %s
-// CHECK-CC2: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // 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{{$}}
-//
-// Runs completion at void ^.
-// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:8 %s -o - | FileCheck -check-prefix=CHECK-CC3 %s
-// CHECK-CC3-NOT: COMPLETION: Pattern : int ttt(bool param, int x = 3) const override{{$}}
-// CHECK-CC3-NOT: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
-// CHECK-CC3-NOT: COMPLETION: Pattern : void vfunc(bool param) override{{$}}
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -37,6 +37,7 @@
 #include "llvm/Support/Path.h"
 #include 
 #include 
+#include 
 #include 
 
 using namespace clang;
@@ -1828,19 +1829,6 @@
   Results.AddResult(CodeCompletionResult(Builder.TakeString()));
 }
 
-static void printOverrideString(llvm::raw_ostream ,
-CodeCompletionString *CCS) {
-  for (const auto  : *CCS) {
-if (C.Kind == CodeCompletionString::CK_Optional)
-  printOverrideString(OS, C.Optional);
-else
-  OS << C.Text;
-// Add a space after return type.
-if (C.Kind == CodeCompletionString::CK_ResultType)
-  OS << ' ';
-  }
-}
-
 static void AddOverrideResults(ResultBuilder ,
const CodeCompletionContext ,
CodeCompletionBuilder ) {
@@ -3162,19 +3150,49 @@
   PP, Ctx, Result, IncludeBriefComments, CCContext, Policy);
 }
 
+static void printOverrideString(const CodeCompletionString ,
+   std::string ,
+   std::string ) {
+  bool SeenTypedChunk = false;
+  for (auto  : CCS) {
+if (Chunk.Kind == CodeCompletionString::CK_Optional) {
+  assert(SeenTypedChunk && "optional parameter before name");
+  // Note that we put all chunks inside into NameAndSignature.
+  printOverrideString(*Chunk.Optional, NameAndSignature,
+ NameAndSignature);
+  continue;
+}
+if (!SeenTypedChunk && Chunk.Kind == CodeCompletionString::CK_TypedText)
+  SeenTypedChunk = true;
+if (SeenTypedChunk)
+  NameAndSignature += Chunk.Text;
+else
+  BeforeName += Chunk.Text;
+// Add a space after return type.
+if (Chunk.Kind == CodeCompletionString::CK_ResultType) {
+  assert(!SeenTypedChunk);
+  BeforeName += " ";
+}
+  }
+}
+
 CodeCompletionString *
 CodeCompletionResult::createCodeCompletionStringForOverride(
 Preprocessor , ASTContext , CodeCompletionBuilder ,
 bool IncludeBriefComments, const CodeCompletionContext ,
 PrintingPolicy ) {
-  std::string OverrideSignature;
-  llvm::raw_string_ostream OS(OverrideSignature);
   auto *CCS = createCodeCompletionStringForDecl(PP, Ctx, Result,
 /*IncludeBriefComments=*/false,
 CCContext, Policy);
-  printOverrideString(OS, CCS);
-  OS << " override";
-  Result.AddTypedTextChunk(Result.getAllocator().CopyString(OS.str()));
+  std::string BeforeName;
+  std::string NameAndSignature;
+  // For overrides all chunks go into the 

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Summarizing the offline discussion, the final results we want in the long run 
is a completion item of the form:

- Displayed to the user: `override foo(int a, int b)`
- Inserted into the editor: `return_type foo(int a, int b) override`
- Filtered by `override foo` (that allows to filter with `override` or `foo`, 
giving reasonably good ranking)

It's **almost** possible to achieve this with the current abstractions, but we 
don't have completion string chunks that are printed but not shown to the user 
(we need those for `return_type` and `override` at the of the completion label).


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

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

[Ilya Biryukov via Phabricator via cfe-commits]

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

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3154
+/// The result contains only 'typed text' and 'text' chunks.
+static void PrintOverrideString(const CodeCompletionString ,
+CodeCompletionBuilder ) {

change name to `printOverrideString`



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);

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



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3182
+  Result.AddChunk(CodeCompletionString::CK_HorizontalSpace);
+  Result.AddTextChunk("override");
   return Result.TakeString();

I believe we should  make `override` a typed text as well.



Comment at: clang/test/CodeCompletion/overrides.cpp:23
 //
-// Runs completion at vo^id.
-// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:5 %s -o - | 
FileCheck -check-prefix=CHECK-CC2 %s
-// CHECK-CC2: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
+// Runs completion at v^oid.
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:4 %s -o - | 
FileCheck -check-prefix=CHECK-CC2 %s

either restore this to original version(if we decide to make return type a 
typed_text as well) or get rid of the void keyword completely and only keep 
`v`? because at first sight it looks like we are triggering on return type :D



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{{$}}

why move this ?


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

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 200927.
ilya-biryukov added a comment.

- Simplify negative tests a bit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62298

Files:
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/overrides.cpp


Index: clang/test/CodeCompletion/overrides.cpp
===
--- clang/test/CodeCompletion/overrides.cpp
+++ clang/test/CodeCompletion/overrides.cpp
@@ -20,14 +20,16 @@
 // CHECK-CC1: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // CHECK-CC1-NOT: COMPLETION: Pattern : void vfunc(bool param) override{{$}}
 //
-// Runs completion at vo^id.
-// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:5 %s -o - | 
FileCheck -check-prefix=CHECK-CC2 %s
-// CHECK-CC2: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
+// Runs completion at v^oid.
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:4 %s -o - | 
FileCheck -check-prefix=CHECK-CC2 %s
 // 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{{$}}
 //
+// Runs completion at vo^id.
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:5 %s -o - | 
FileCheck -check-prefix=CHECK-CC3 %s
+// CHECK-CC3-NOT: override
+//
 // Runs completion at void ^.
-// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:8 %s -o - | 
FileCheck -check-prefix=CHECK-CC3 %s
-// CHECK-CC3-NOT: COMPLETION: Pattern : int ttt(bool param, int x = 3) const 
override{{$}}
-// CHECK-CC3-NOT: COMPLETION: Pattern : void vfunc(bool param, int p) 
override{{$}}
-// CHECK-CC3-NOT: COMPLETION: Pattern : void vfunc(bool param) override{{$}}
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:8 %s -o - | 
FileCheck -check-prefix=CHECK-CC4 %s
+// CHECK-CC4-NOT: override
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -1828,19 +1828,6 @@
   Results.AddResult(CodeCompletionResult(Builder.TakeString()));
 }
 
-static void printOverrideString(llvm::raw_ostream ,
-CodeCompletionString *CCS) {
-  for (const auto  : *CCS) {
-if (C.Kind == CodeCompletionString::CK_Optional)
-  printOverrideString(OS, C.Optional);
-else
-  OS << C.Text;
-// Add a space after return type.
-if (C.Kind == CodeCompletionString::CK_ResultType)
-  OS << ' ';
-  }
-}
-
 static void AddOverrideResults(ResultBuilder ,
const CodeCompletionContext ,
CodeCompletionBuilder ) {
@@ -3162,19 +3149,37 @@
   PP, Ctx, Result, IncludeBriefComments, CCContext, Policy);
 }
 
+/// Turns a completion string for a declaration into a textual representation.
+/// The result contains only 'typed text' and 'text' chunks.
+static void PrintOverrideString(const CodeCompletionString ,
+CodeCompletionBuilder ) {
+  for (auto  : CCS) {
+if (Chunk.Kind == CodeCompletionString::CK_Optional) {
+  PrintOverrideString(*Chunk.Optional, Result);
+  continue;
+}
+if (Chunk.Kind == CodeCompletionString::CK_TypedText)
+  Result.AddTypedTextChunk(Chunk.Text);
+else
+  Result.AddTextChunk(Chunk.Text);
+// Add a space after return type.
+if (Chunk.Kind == CodeCompletionString::CK_ResultType)
+  Result.AddChunk(CodeCompletionString::CK_HorizontalSpace);
+  }
+}
+
 CodeCompletionString *
 CodeCompletionResult::createCodeCompletionStringForOverride(
 Preprocessor , ASTContext , CodeCompletionBuilder ,
 bool IncludeBriefComments, const CodeCompletionContext ,
 PrintingPolicy ) {
-  std::string OverrideSignature;
-  llvm::raw_string_ostream OS(OverrideSignature);
   auto *CCS = createCodeCompletionStringForDecl(PP, Ctx, Result,
 /*IncludeBriefComments=*/false,
 CCContext, Policy);
-  printOverrideString(OS, CCS);
-  OS << " override";
-  Result.AddTypedTextChunk(Result.getAllocator().CopyString(OS.str()));
+  // For overrides all chunks go into the result, none are informative.
+  PrintOverrideString(*CCS, Result);
+  Result.AddChunk(CodeCompletionString::CK_HorizontalSpace);
+  Result.AddTextChunk("override");
   return Result.TakeString();
 }
 


Index: clang/test/CodeCompletion/overrides.cpp
===
--- clang/test/CodeCompletion/overrides.cpp
+++ clang/test/CodeCompletion/overrides.cpp
@@ -20,14 +20,16 @@
 // CHECK-CC1: COMPLETION: Pattern : void vfunc(bool param, int p) override{{$}}
 // CHECK-CC1-NOT: COMPLETION: 

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Sorry for so many changes, this is ready for review now!


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