[PATCH] D51102: [clangd] Move function argument snippet disable mechanism from LSP rendering to internal clangd reprensentation.

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL340527: [clangd] Move function argument snippet disable 
mechanism from LSP rendering to… (authored by kadircet, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D51102

Files:
  clang-tools-extra/trunk/clangd/CodeComplete.cpp
  clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp

Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -1140,7 +1140,7 @@
   // For now we just return one of the doc strings arbitrarily.
   EXPECT_THAT(A.Documentation, AnyOf(HasSubstr("Overload with int"),
  HasSubstr("Overload with bool")));
-  EXPECT_EQ(A.SnippetSuffix, "(${0})");
+  EXPECT_EQ(A.SnippetSuffix, "($0)");
 }
 
 TEST(CompletionTest, DocumentationFromChangedFileCrash) {
@@ -1648,55 +1648,35 @@
 SigDoc("Doc from sema";
 }
 
-TEST(CompletionTest, RenderWithSnippetsForFunctionArgsDisabled) {
+TEST(CompletionTest, CompletionFunctionArgsDisabled) {
   CodeCompleteOptions Opts;
-  Opts.EnableFunctionArgSnippets = true;
-  {
-CodeCompletion C;
-C.RequiredQualifier = "Foo::";
-C.Name = "x";
-C.SnippetSuffix = "()";
-
-auto R = C.render(Opts);
-EXPECT_EQ(R.textEdit->newText, "Foo::x");
-EXPECT_EQ(R.insertTextFormat, InsertTextFormat::PlainText);
-  }
-
   Opts.EnableSnippets = true;
   Opts.EnableFunctionArgSnippets = false;
+  const std::string Header =
+  R"cpp(
+  void xfoo();
+  void xfoo(int x, int y);
+  void xbar();
+  void f() {
+)cpp";
   {
-CodeCompletion C;
-C.RequiredQualifier = "Foo::";
-C.Name = "x";
-C.SnippetSuffix = "";
-
-auto R = C.render(Opts);
-EXPECT_EQ(R.textEdit->newText, "Foo::x");
-EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+auto Results = completions(Header + "\nxfo^", {}, Opts);
+EXPECT_THAT(
+Results.Completions,
+UnorderedElementsAre(AllOf(Named("xfoo"), SnippetSuffix("()")),
+ AllOf(Named("xfoo"), SnippetSuffix("($0)";
   }
-
   {
-CodeCompletion C;
-C.RequiredQualifier = "Foo::";
-C.Name = "x";
-C.SnippetSuffix = "()";
-C.Kind = CompletionItemKind::Method;
-
-auto R = C.render(Opts);
-EXPECT_EQ(R.textEdit->newText, "Foo::x()");
-EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+auto Results = completions(Header + "\nxba^", {}, Opts);
+EXPECT_THAT(Results.Completions, UnorderedElementsAre(AllOf(
+ Named("xbar"), SnippetSuffix("()";
   }
-
   {
-CodeCompletion C;
-C.RequiredQualifier = "Foo::";
-C.Name = "x";
-C.SnippetSuffix = "(${0:bool})";
-C.Kind = CompletionItemKind::Function;
-
-auto R = C.render(Opts);
-EXPECT_EQ(R.textEdit->newText, "Foo::x(${0})");
-EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+Opts.BundleOverloads = true;
+auto Results = completions(Header + "\nxfo^", {}, Opts);
+EXPECT_THAT(
+Results.Completions,
+UnorderedElementsAre(AllOf(Named("xfoo"), SnippetSuffix("($0)";
   }
 }
 
Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -269,7 +269,8 @@
 CodeCompletionString *SemaCCS,
 const IncludeInserter , StringRef FileName,
 const CodeCompleteOptions )
-  : ASTCtx(ASTCtx), ExtractDocumentation(Opts.IncludeComments) {
+  : ASTCtx(ASTCtx), ExtractDocumentation(Opts.IncludeComments),
+EnableFunctionArgSnippets(Opts.EnableFunctionArgSnippets) {
 add(C, SemaCCS);
 if (C.SemaResult) {
   Completion.Origin |= SymbolOrigin::AST;
@@ -385,10 +386,17 @@
   }
 
   std::string summarizeSnippet() const {
-if (auto *Snippet = onlyValue<::SnippetSuffix>())
-  return *Snippet;
-// All bundles are function calls.
-return "(${0})";
+auto *Snippet = onlyValue<::SnippetSuffix>();
+if (!Snippet)
+  // All bundles are function calls.
+  return "($0)";
+if (!Snippet->empty() && !EnableFunctionArgSnippets &&
+((Completion.Kind == CompletionItemKind::Function) ||
+ (Completion.Kind == CompletionItemKind::Method)) &&
+(Snippet->front() == '(') && (Snippet->back() == ')'))
+  // Check whether function has any parameters or not.
+  return Snippet->size() > 2 ? "($0)" : "()";
+return *Snippet;
   }
 
   std::string summarizeSignature() const {
@@ -402,6 +410,7 @@
   CodeCompletion Completion;
   SmallVector 

[PATCH] D51102: [clangd] Move function argument snippet disable mechanism from LSP rendering to internal clangd reprensentation.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51102



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

[PATCH] D51102: [clangd] Move function argument snippet disable mechanism from LSP rendering to internal clangd reprensentation.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added reviewers: ilya-biryukov, ioeric, hokein.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay.

We were handling the EnableFunctionArgSnippets only when we are producing LSP
response. Move that code into CompletionItem generation so that internal clients
can benefit from that as well.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51102

Files:
  clangd/CodeComplete.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1140,7 +1140,7 @@
   // For now we just return one of the doc strings arbitrarily.
   EXPECT_THAT(A.Documentation, AnyOf(HasSubstr("Overload with int"),
  HasSubstr("Overload with bool")));
-  EXPECT_EQ(A.SnippetSuffix, "(${0})");
+  EXPECT_EQ(A.SnippetSuffix, "($0)");
 }
 
 TEST(CompletionTest, DocumentationFromChangedFileCrash) {
@@ -1648,55 +1648,35 @@
 SigDoc("Doc from sema";
 }
 
-TEST(CompletionTest, RenderWithSnippetsForFunctionArgsDisabled) {
+TEST(CompletionTest, CompletionFunctionArgsDisabled) {
   CodeCompleteOptions Opts;
-  Opts.EnableFunctionArgSnippets = true;
-  {
-CodeCompletion C;
-C.RequiredQualifier = "Foo::";
-C.Name = "x";
-C.SnippetSuffix = "()";
-
-auto R = C.render(Opts);
-EXPECT_EQ(R.textEdit->newText, "Foo::x");
-EXPECT_EQ(R.insertTextFormat, InsertTextFormat::PlainText);
-  }
-
   Opts.EnableSnippets = true;
   Opts.EnableFunctionArgSnippets = false;
+  const std::string Header =
+  R"cpp(
+  void xfoo();
+  void xfoo(int x, int y);
+  void xbar();
+  void f() {
+)cpp";
   {
-CodeCompletion C;
-C.RequiredQualifier = "Foo::";
-C.Name = "x";
-C.SnippetSuffix = "";
-
-auto R = C.render(Opts);
-EXPECT_EQ(R.textEdit->newText, "Foo::x");
-EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+auto Results = completions(Header + "\nxfo^", {}, Opts);
+EXPECT_THAT(
+Results.Completions,
+UnorderedElementsAre(AllOf(Named("xfoo"), SnippetSuffix("()")),
+ AllOf(Named("xfoo"), SnippetSuffix("($0)";
   }
-
   {
-CodeCompletion C;
-C.RequiredQualifier = "Foo::";
-C.Name = "x";
-C.SnippetSuffix = "()";
-C.Kind = CompletionItemKind::Method;
-
-auto R = C.render(Opts);
-EXPECT_EQ(R.textEdit->newText, "Foo::x()");
-EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+auto Results = completions(Header + "\nxba^", {}, Opts);
+EXPECT_THAT(Results.Completions, UnorderedElementsAre(AllOf(
+ Named("xbar"), SnippetSuffix("()";
   }
-
   {
-CodeCompletion C;
-C.RequiredQualifier = "Foo::";
-C.Name = "x";
-C.SnippetSuffix = "(${0:bool})";
-C.Kind = CompletionItemKind::Function;
-
-auto R = C.render(Opts);
-EXPECT_EQ(R.textEdit->newText, "Foo::x(${0})");
-EXPECT_EQ(R.insertTextFormat, InsertTextFormat::Snippet);
+Opts.BundleOverloads = true;
+auto Results = completions(Header + "\nxfo^", {}, Opts);
+EXPECT_THAT(
+Results.Completions,
+UnorderedElementsAre(AllOf(Named("xfoo"), SnippetSuffix("($0)";
   }
 }
 
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -269,7 +269,8 @@
 CodeCompletionString *SemaCCS,
 const IncludeInserter , StringRef FileName,
 const CodeCompleteOptions )
-  : ASTCtx(ASTCtx), ExtractDocumentation(Opts.IncludeComments) {
+  : ASTCtx(ASTCtx), ExtractDocumentation(Opts.IncludeComments),
+EnableFunctionArgSnippets(Opts.EnableFunctionArgSnippets) {
 add(C, SemaCCS);
 if (C.SemaResult) {
   Completion.Origin |= SymbolOrigin::AST;
@@ -385,10 +386,17 @@
   }
 
   std::string summarizeSnippet() const {
-if (auto *Snippet = onlyValue<::SnippetSuffix>())
-  return *Snippet;
-// All bundles are function calls.
-return "(${0})";
+auto *Snippet = onlyValue<::SnippetSuffix>();
+if (!Snippet)
+  // All bundles are function calls.
+  return "($0)";
+if (!Snippet->empty() && !EnableFunctionArgSnippets &&
+((Completion.Kind == CompletionItemKind::Function) ||
+ (Completion.Kind == CompletionItemKind::Method)) &&
+(Snippet->front() == '(') && (Snippet->back() == ')'))
+  // Check whether function has any parameters or not.
+  return Snippet->size() > 2 ? "($0)" : "()";
+return *Snippet;
   }
 
   std::string summarizeSignature() const {
@@ -402,6 +410,7 @@
   CodeCompletion Completion;
   SmallVector Bundled;
   bool ExtractDocumentation;
+  bool EnableFunctionArgSnippets;
 };
 
 // Determine