[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-09-02 Thread Raoul Wols via Phabricator via cfe-commits
rwols updated this revision to Diff 113657.
rwols added a comment.

- Adjust SnippetCompletionItemCollector such that it only sets insertTextFormat 
to InsertTextFormat::Snippet when it's actually needed (i.e. we encounter a 
CK_Placeholder chunk).
- Fix failing tests in test/clangd/{completion.test,authority-less-uri.test} 
because of the new changes.
- Add new tests in test/clangd/completion-snippet.test that tests the snippet 
functionality.


https://reviews.llvm.org/D37101

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/tool/ClangdMain.cpp
  test/clangd/authority-less-uri.test
  test/clangd/completion-snippet.test
  test/clangd/completion.test

Index: test/clangd/completion.test
===
--- test/clangd/completion.test
+++ test/clangd/completion.test
@@ -16,25 +16,25 @@
 # nondeterministic, so we check regardless of order.
 #
 # CHECK: {"jsonrpc":"2.0","id":1,"result":[
-# CHECK-DAG: {"label":"a","kind":5,"detail":"int","sortText":"00035a","filterText":"a","insertText":"a"}
-# CHECK-DAG: {"label":"bb","kind":5,"detail":"int","sortText":"00035bb","filterText":"bb","insertText":"bb"}
-# CHECK-DAG: {"label":"ccc","kind":5,"detail":"int","sortText":"00035ccc","filterText":"ccc","insertText":"ccc"}
-# CHECK-DAG: {"label":"operator=(const fake &)","kind":2,"detail":"fake &","sortText":"00034operator=","filterText":"operator=","insertText":"operator="}
-# CHECK-DAG: {"label":"~fake()","kind":4,"detail":"void","sortText":"00034~fake","filterText":"~fake","insertText":"~fake"}
-# CHECK-DAG: {"label":"f(int i, const float f) const","kind":2,"detail":"int","sortText":"00035f","filterText":"f","insertText":"f"}
+# CHECK-DAG: {"label":"a","kind":5,"detail":"int","sortText":"00035a","filterText":"a","insertText":"a","insertTextFormat":1}
+# CHECK-DAG: {"label":"bb","kind":5,"detail":"int","sortText":"00035bb","filterText":"bb","insertText":"bb","insertTextFormat":1}
+# CHECK-DAG: {"label":"ccc","kind":5,"detail":"int","sortText":"00035ccc","filterText":"ccc","insertText":"ccc","insertTextFormat":1}
+# CHECK-DAG: {"label":"operator=(const fake &)","kind":2,"detail":"fake &","sortText":"00034operator=","filterText":"operator=","insertText":"operator=","insertTextFormat":1}
+# CHECK-DAG: {"label":"~fake()","kind":4,"detail":"void","sortText":"00034~fake","filterText":"~fake","insertText":"~fake","insertTextFormat":1}
+# CHECK-DAG: {"label":"f(int i, const float f) const","kind":2,"detail":"int","sortText":"00035f","filterText":"f","insertText":"f","insertTextFormat":1}
 # CHECK: ]}
 Content-Length: 148
 
 {"jsonrpc":"2.0","id":2,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":3,"character":5}}}
 # Repeat the completion request, expect the same results.
 #
 # CHECK: {"jsonrpc":"2.0","id":2,"result":[
-# CHECK-DAG: {"label":"a","kind":5,"detail":"int","sortText":"00035a","filterText":"a","insertText":"a"}
-# CHECK-DAG: {"label":"bb","kind":5,"detail":"int","sortText":"00035bb","filterText":"bb","insertText":"bb"}
-# CHECK-DAG: {"label":"ccc","kind":5,"detail":"int","sortText":"00035ccc","filterText":"ccc","insertText":"ccc"}
-# CHECK-DAG: {"label":"operator=(const fake &)","kind":2,"detail":"fake &","sortText":"00034operator=","filterText":"operator=","insertText":"operator="}
-# CHECK-DAG: {"label":"~fake()","kind":4,"detail":"void","sortText":"00034~fake","filterText":"~fake","insertText":"~fake"}
-# CHECK-DAG: {"label":"f(int i, const float f) const","kind":2,"detail":"int","sortText":"00035f","filterText":"f","insertText":"f"}
+# CHECK-DAG: {"label":"a","kind":5,"detail":"int","sortText":"00035a","filterText":"a","insertText":"a","insertTextFormat":1}
+# CHECK-DAG: {"label":"bb","kind":5,"detail":"int","sortText":"00035bb","filterText":"bb","insertText":"bb","insertTextFormat":1}
+# CHECK-DAG: {"label":"ccc","kind":5,"detail":"int","sortText":"00035ccc","filterText":"ccc","insertText":"ccc","insertTextFormat":1}
+# CHECK-DAG: {"label":"operator=(const fake &)","kind":2,"detail":"fake &","sortText":"00034operator=","filterText":"operator=","insertText":"operator=","insertTextFormat":1}
+# CHECK-DAG: {"label":"~fake()","kind":4,"detail":"void","sortText":"00034~fake","filterText":"~fake","insertText":"~fake","insertTextFormat":1}
+# CHECK-DAG: {"label":"f(int i, const float f) const","kind":2,"detail":"int","sortText":"00035f","filterText":"f","insertText":"f","insertTextFormat":1}
 # CHECK: ]}
 # Update the source file and check for completions again.
 Content-Length: 226
@@ -47,7 +47,7 @@
 # Repeat the completion request, expect the same results.
 #
 # CHECK: {"jsonrpc":"2.0","id":3,"result":[
-# CHECK-DAG: {"label":"func()","kind":2,"detail":"int (*)(int, int)","sortText":"00034func","filterText":"func","insertText":"func"}
+# CHECK-DAG: 

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-09-02 Thread Raoul Wols via Phabricator via cfe-commits
rwols marked 4 inline comments as done.
rwols added inline comments.



Comment at: clangd/ClangdUnit.cpp:419
+assert(item.detail.empty() && "Unexpected extraneous CK_ResultType");
+Item.detail = Chunk.Text;
+break;

ilya-biryukov wrote:
> Won't that assertion fail with function return types? Let's add a test for 
> that.
> 
> ```
> int (*foo(int a))(float);
> ```
I'll make sure to add a test case for this!


https://reviews.llvm.org/D37101



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


[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-09-02 Thread Raoul Wols via Phabricator via cfe-commits
rwols added a comment.

Forgot to mention I've also made sure the `filterText` field is taken care of 
now (both for snippet and for plaintext items).


https://reviews.llvm.org/D37101



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


[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-09-02 Thread Raoul Wols via Phabricator via cfe-commits
rwols updated this revision to Diff 113650.
rwols marked 3 inline comments as done.
rwols added a comment.

- Split up the CompletionItemsCollector into two classes called 
PlainTextCompletionItemsCollector and SnippetCompletionItemsCollector to allow 
collecting both types of items.
- Add a command-line setting to clangd called "--enable-snippets" that, when 
set, will make clangd use the SnippetCompletionItemsCollector. The default is 
to use the PlainTextCompletionItemsCollector.
- Enable "code pattern" completions when "--enable-snippets" is true.
- Do not add a space in the completion label when we encounter a 
CK_VerticalSpace in the SnippetCompletionItemsCollector. A space looks weird.

I will add tests now.


https://reviews.llvm.org/D37101

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/tool/ClangdMain.cpp

Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -26,6 +26,13 @@
llvm::cl::desc("Number of async workers used by clangd"),
llvm::cl::init(getDefaultAsyncThreadsCount()));
 
+// FIXME: Make snippets the default once the client landscape has adjusted.
+static llvm::cl::opt EnableSnippets(
+"enable-snippets",
+llvm::cl::desc("Present completions with embedded snippets instead of "
+   "plaintext completions"),
+llvm::cl::init(false));
+
 static llvm::cl::opt RunSynchronously(
 "run-synchronously",
 llvm::cl::desc("Parse on main thread. If set, -j is ignored"),
@@ -61,6 +68,7 @@
   if (!ResourceDir.empty())
 ResourceDirRef = ResourceDir;
 
-  ClangdLSPServer LSPServer(Out, WorkerThreadsCount, ResourceDirRef);
+  ClangdLSPServer LSPServer(Out, WorkerThreadsCount, EnableSnippets,
+ResourceDirRef);
   LSPServer.run(std::cin);
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -389,6 +389,9 @@
   /// themselves.
   std::vector additionalTextEdits;
 
+  CompletionItem() = default;
+  CompletionItem(const InsertTextFormat Format);
+
   // TODO(krasimir): The following optional fields defined by the language
   // server protocol are unsupported:
   //
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -705,6 +705,9 @@
   return Result;
 }
 
+CompletionItem::CompletionItem(const InsertTextFormat Format)
+: insertTextFormat(Format) {}
+
 std::string CompletionItem::unparse(const CompletionItem ) {
   std::string Result = "{";
   llvm::raw_string_ostream Os(Result);
@@ -724,8 +727,8 @@
   if (!CI.insertText.empty())
 Os << R"("insertText":")" << llvm::yaml::escape(CI.insertText) << R"(",)";
   if (CI.insertTextFormat != InsertTextFormat::Missing) {
-Os << R"("insertTextFormat":")" << static_cast(CI.insertTextFormat)
-   << R"(",)";
+Os << R"("insertTextFormat":)" << static_cast(CI.insertTextFormat)
+   << R"(,)";
   }
   if (CI.textEdit)
 Os << R"("textEdit":)" << TextEdit::unparse(*CI.textEdit) << ',';
Index: clangd/ClangdUnit.h
===
--- clangd/ClangdUnit.h
+++ clangd/ClangdUnit.h
@@ -253,7 +253,8 @@
 codeComplete(PathRef FileName, tooling::CompileCommand Command,
  PrecompiledPreamble const *Preamble, StringRef Contents,
  Position Pos, IntrusiveRefCntPtr VFS,
- std::shared_ptr PCHs);
+ std::shared_ptr PCHs,
+ const bool SnippetCompletions);
 
 /// Get definition of symbol at a specified \p Pos.
 std::vector findDefinitions(ParsedAST , Position Pos);
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -272,68 +272,277 @@
   }
 }
 
+std::string SnippetEscape(const llvm::StringRef Text) {
+  std::string Result;
+  Result.reserve(Text.size()); // Assume '$', '}' and '\\' are rare.
+  for (const auto Character : Text) {
+if (Character == '$' || Character == '}' || Character == '\\')
+  Result.push_back('\\');
+Result.push_back(Character);
+  }
+  return Result;
+}
+
 class CompletionItemsCollector : public CodeCompleteConsumer {
-  std::vector *Items;
-  std::shared_ptr Allocator;
-  CodeCompletionTUInfo CCTUInfo;
 
 public:
-  CompletionItemsCollector(std::vector *Items,
-   const CodeCompleteOptions )
+  CompletionItemsCollector(const CodeCompleteOptions ,
+   std::vector )
   : CodeCompleteConsumer(CodeCompleteOpts, /*OutputIsBinary=*/false),
 Items(Items),
 Allocator(std::make_shared()),
 

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-09-01 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments.



Comment at: clangd/ClangdUnit.cpp:337
+// Fill in the kind field of the CompletionItem.
+Item.kind = getKind(Result.CursorKind);
+

ilya-biryukov wrote:
> ilya-biryukov wrote:
> > Could we also set `Item.filterText` to completion item name?
> > So that various pieces of function signature would not match on user input.
> After some investigation: `CK_TypedText` chunks are exactly the things that 
> should go into `filterText`.
> Also, VSCode seems to ignore `filterText` right now, but that's certainly a 
> bug in VSCode, I still propose to set `filterText` properly.
Sorry for confusion, after updating VSCode to the last version, `filterText` 
works just fine.


https://reviews.llvm.org/D37101



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


[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-09-01 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments.



Comment at: clangd/ClangdUnit.cpp:337
+// Fill in the kind field of the CompletionItem.
+Item.kind = getKind(Result.CursorKind);
+

ilya-biryukov wrote:
> Could we also set `Item.filterText` to completion item name?
> So that various pieces of function signature would not match on user input.
After some investigation: `CK_TypedText` chunks are exactly the things that 
should go into `filterText`.
Also, VSCode seems to ignore `filterText` right now, but that's certainly a bug 
in VSCode, I still propose to set `filterText` properly.


https://reviews.llvm.org/D37101



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


[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-09-01 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments.



Comment at: clangd/ClangdUnit.cpp:418
+// for functions and methods, the return type.
+assert(item.detail.empty() && "Unexpected extraneous CK_ResultType");
+Item.detail = Chunk.Text;

Typo: should be `Item` instead of `item`.

Otherwise this does not compile with assertions enabled.


https://reviews.llvm.org/D37101



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


[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-09-01 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment.

Also wanted to stress once again that we need some tests for this change.


https://reviews.llvm.org/D37101



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


[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-09-01 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment.

This looks like a useful change even without prior changes to VSCode.

Maybe add a command-line flag to clangd(`--enable-snippets`) and commit that?
When snippets are disabled, we could simply do `insertText =  /**/` 
after `ProcessChunks`, we will deprecate and remove that flag eventually when 
all the clients support it.




Comment at: clangd/ClangdUnit.cpp:337
+// Fill in the kind field of the CompletionItem.
+Item.kind = getKind(Result.CursorKind);
+

Could we also set `Item.filterText` to completion item name?
So that various pieces of function signature would not match on user input.



Comment at: clangd/ClangdUnit.cpp:419
+assert(item.detail.empty() && "Unexpected extraneous CK_ResultType");
+Item.detail = Chunk.Text;
+break;

Won't that assertion fail with function return types? Let's add a test for that.

```
int (*foo(int a))(float);
```


https://reviews.llvm.org/D37101



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


[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-08-30 Thread Raoul Wols via Phabricator via cfe-commits
rwols updated this revision to Diff 113258.
rwols added a comment.

Some more tweaks

- Move assert to constructor of CompletionItemsCollector
- Use a local variable for the annotations count
- Move the documentation handling to its own private const member function


https://reviews.llvm.org/D37101

Files:
  clangd/ClangdUnit.cpp
  clangd/Protocol.cpp

Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -724,8 +724,8 @@
   if (!CI.insertText.empty())
 Os << R"("insertText":")" << llvm::yaml::escape(CI.insertText) << R"(",)";
   if (CI.insertTextFormat != InsertTextFormat::Missing) {
-Os << R"("insertTextFormat":")" << static_cast(CI.insertTextFormat)
-   << R"(",)";
+Os << R"("insertTextFormat":)" << static_cast(CI.insertTextFormat)
+   << R"(,)";
   }
   if (CI.textEdit)
 Os << R"("textEdit":)" << TextEdit::unparse(*CI.textEdit) << ',';
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -272,6 +272,17 @@
   }
 }
 
+std::string SnippetEscape(const llvm::StringRef Text) {
+  std::string Result;
+  Result.reserve(Text.size()); // Assume '$', '}' and '\\' are rare.
+  for (const auto Character : Text) {
+if (Character == '$' || Character == '}' || Character == '\\')
+  Result.push_back('\\');
+Result.push_back(Character);
+  }
+  return Result;
+}
+
 class CompletionItemsCollector : public CodeCompleteConsumer {
   std::vector *Items;
   std::shared_ptr Allocator;
@@ -283,50 +294,178 @@
   : CodeCompleteConsumer(CodeCompleteOpts, /*OutputIsBinary=*/false),
 Items(Items),
 Allocator(std::make_shared()),
-CCTUInfo(Allocator) {}
+CCTUInfo(Allocator) {
+assert(Items && "We need a non-null items container here");
+  }
 
   void ProcessCodeCompleteResults(Sema , CodeCompletionContext Context,
   CodeCompletionResult *Results,
   unsigned NumResults) override {
-for (unsigned I = 0; I != NumResults; ++I) {
-  CodeCompletionResult  = Results[I];
-  CodeCompletionString *CCS = Result.CreateCodeCompletionString(
+Items->reserve(NumResults);
+for (unsigned I = 0; I < NumResults; ++I) {
+  auto  = Results[I];
+  const auto *CCS = Result.CreateCodeCompletionString(
   S, Context, *Allocator, CCTUInfo,
   CodeCompleteOpts.IncludeBriefComments);
-  if (CCS) {
-CompletionItem Item;
-for (CodeCompletionString::Chunk C : *CCS) {
-  switch (C.Kind) {
-  case CodeCompletionString::CK_ResultType:
-Item.detail = C.Text;
-break;
-  case CodeCompletionString::CK_Optional:
-break;
-  default:
-Item.label += C.Text;
-break;
-  }
-}
-assert(CCS->getTypedText());
-Item.kind = getKind(Result.CursorKind);
-// Priority is a 16-bit integer, hence at most 5 digits.
-assert(CCS->getPriority() < 9 && "Expecting code completion result "
- "priority to have at most "
- "5-digits");
-llvm::raw_string_ostream(Item.sortText)
-<< llvm::format("%05d%s", CCS->getPriority(), CCS->getTypedText());
-Item.insertText = Item.filterText = CCS->getTypedText();
-if (CCS->getBriefComment())
-  Item.documentation = CCS->getBriefComment();
-Items->push_back(std::move(Item));
-  }
+  assert(CCS && "Expected the CodeCompletionString to be non-null");
+  Items->push_back(ProcessCodeCompleteResult(Result, *CCS));
 }
   }
 
   GlobalCodeCompletionAllocator () override { return *Allocator; }
 
   CodeCompletionTUInfo () override { return CCTUInfo; }
-};
+
+private:
+  CompletionItem
+  ProcessCodeCompleteResult(const CodeCompletionResult ,
+const CodeCompletionString ) const {
+CompletionItem Item;
+
+// Always a snippet. This is because CK_Informative chunks should not be
+// inserted into the text buffer, but they are part of the label. For
+// example, "foo::bar() const" is the label, but "bar()" is the insertText.
+Item.insertTextFormat = InsertTextFormat::Snippet;
+
+// Fill in the documentation field of the CompletionItem.
+ProcessDocumentation(CCS, Item);
+
+// Fill in the label, detail and insertText fields of the CompletionItem.
+ProcessChunks(CCS, Item);
+
+// Fill in the kind field of the CompletionItem.
+Item.kind = getKind(Result.CursorKind);
+
+// Fill in the sortText of the CompletionItem.
+assert(CCS.getPriority() < 9 && "Expecting code completion result "
+"priority to have at most 5-digits");
+llvm::raw_string_ostream(Item.sortText)
+ 

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-08-30 Thread Krasimir Georgiev via Phabricator via cfe-commits
krasimir added inline comments.



Comment at: clangd/ClangdUnit.cpp:302
   unsigned NumResults) override {
-for (unsigned I = 0; I != NumResults; ++I) {
-  CodeCompletionResult  = Results[I];
-  CodeCompletionString *CCS = Result.CreateCodeCompletionString(
+assert(Items && "We need a non-null items container here");
+Items->reserve(NumResults);

Move this assert in the `CompletionItemsCollector` constructor.



Comment at: clangd/ClangdUnit.cpp:322
+const CodeCompletionString ) const {
+// The object that we'll return.
+CompletionItem Item;

This comment is unnecessary.



Comment at: clangd/ClangdUnit.cpp:339
+  }
+  for (unsigned j = 0; j < CCS.getAnnotationCount(); ++j) {
+Item.documentation += CCS.getAnnotation(j);

In LLVM, prefer using an explicit variable for the limit of iteration. In this 
case, since you're using `getAnnotationCount` before, you could directly put it 
into a variable above.


https://reviews.llvm.org/D37101



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


[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-08-30 Thread Raoul Wols via Phabricator via cfe-commits
rwols added a comment.

So at this point the C++ changes are basically done, save for some minor things 
I guess. The problem is still that the VSCode extension doesn't do anything 
clever with the snippets. I have zero experience with TypeScript let alone 
extension development for VSCode, so it can take a while for me to investigate. 
I'd appreciate if someone else could take a look.


https://reviews.llvm.org/D37101



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


[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-08-30 Thread Raoul Wols via Phabricator via cfe-commits
rwols marked 10 inline comments as done.
rwols added a comment.

I followed your advice and kept the snippet functionality. We'll do the 
SignatureHelp stuff in another review.

A "major" change is that, because CK_Informative chunks are put into the label 
now, we have to use the insertText, always.


https://reviews.llvm.org/D37101



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


[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-08-30 Thread Raoul Wols via Phabricator via cfe-commits
rwols updated this revision to Diff 113219.
rwols added a comment.

Tidy up snippet handling

- Put CK_Informative chunks in the label
- Assert that there's at most one CK_ResultType chunk
- CK_CurrentParameter never occurs while collecting completions, only while 
handling overloads.
- For CK_VerticalSpace, use a space for the label (but a newline for the 
insertText).
- Move the SnippetEscape function outside the class and into an unnamed 
namespace.


https://reviews.llvm.org/D37101

Files:
  clangd/ClangdUnit.cpp
  clangd/Protocol.cpp

Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -724,8 +724,8 @@
   if (!CI.insertText.empty())
 Os << R"("insertText":")" << llvm::yaml::escape(CI.insertText) << R"(",)";
   if (CI.insertTextFormat != InsertTextFormat::Missing) {
-Os << R"("insertTextFormat":")" << static_cast(CI.insertTextFormat)
-   << R"(",)";
+Os << R"("insertTextFormat":)" << static_cast(CI.insertTextFormat)
+   << R"(,)";
   }
   if (CI.textEdit)
 Os << R"("textEdit":)" << TextEdit::unparse(*CI.textEdit) << ',';
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -272,6 +272,17 @@
   }
 }
 
+std::string SnippetEscape(const llvm::StringRef Text) {
+  std::string Result;
+  Result.reserve(Text.size()); // Assume '$', '}' and '\\' are rare.
+  for (const auto Character : Text) {
+if (Character == '$' || Character == '}' || Character == '\\')
+  Result.push_back('\\');
+Result.push_back(Character);
+  }
+  return Result;
+}
+
 class CompletionItemsCollector : public CodeCompleteConsumer {
   std::vector *Items;
   std::shared_ptr Allocator;
@@ -288,45 +299,161 @@
   void ProcessCodeCompleteResults(Sema , CodeCompletionContext Context,
   CodeCompletionResult *Results,
   unsigned NumResults) override {
-for (unsigned I = 0; I != NumResults; ++I) {
-  CodeCompletionResult  = Results[I];
-  CodeCompletionString *CCS = Result.CreateCodeCompletionString(
+assert(Items && "We need a non-null items container here");
+Items->reserve(NumResults);
+for (unsigned I = 0; I < NumResults; ++I) {
+  auto  = Results[I];
+  const auto *CCS = Result.CreateCodeCompletionString(
   S, Context, *Allocator, CCTUInfo,
   CodeCompleteOpts.IncludeBriefComments);
-  if (CCS) {
-CompletionItem Item;
-for (CodeCompletionString::Chunk C : *CCS) {
-  switch (C.Kind) {
-  case CodeCompletionString::CK_ResultType:
-Item.detail = C.Text;
-break;
-  case CodeCompletionString::CK_Optional:
-break;
-  default:
-Item.label += C.Text;
-break;
-  }
-}
-assert(CCS->getTypedText());
-Item.kind = getKind(Result.CursorKind);
-// Priority is a 16-bit integer, hence at most 5 digits.
-assert(CCS->getPriority() < 9 && "Expecting code completion result "
- "priority to have at most "
- "5-digits");
-llvm::raw_string_ostream(Item.sortText)
-<< llvm::format("%05d%s", CCS->getPriority(), CCS->getTypedText());
-Item.insertText = Item.filterText = CCS->getTypedText();
-if (CCS->getBriefComment())
-  Item.documentation = CCS->getBriefComment();
-Items->push_back(std::move(Item));
-  }
+  assert(CCS && "Expected the CodeCompletionString to be non-null");
+  Items->push_back(ProcessCodeCompleteResult(Result, *CCS));
 }
   }
 
   GlobalCodeCompletionAllocator () override { return *Allocator; }
 
   CodeCompletionTUInfo () override { return CCTUInfo; }
-};
+
+private:
+  CompletionItem
+  ProcessCodeCompleteResult(const CodeCompletionResult ,
+const CodeCompletionString ) const {
+// The object that we'll return.
+CompletionItem Item;
+
+// Always a snippet. This is because CK_Informative chunks should not be
+// inserted into the text buffer, but they are part of the label. For
+// example, "foo::bar() const" is the label, but "bar()" is the insertText.
+Item.insertTextFormat = InsertTextFormat::Snippet;
+
+// Things like __attribute__((nonnull(1,3))) and [[noreturn]]. Present this
+// information in the documentation field.
+if (CCS.getAnnotationCount() > 0) {
+  Item.documentation += "Annotation";
+  if (CCS.getAnnotationCount() == 1) {
+Item.documentation += ": ";
+  } else /* CCS.getAnnotationCount() > 1 */ {
+Item.documentation += "s: ";
+  }
+  for (unsigned j = 0; j < CCS.getAnnotationCount(); ++j) {
+Item.documentation += CCS.getAnnotation(j);
+Item.documentation += j == 

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-08-28 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments.



Comment at: clangd/ClangdUnit.cpp:321
+
+// Fill in the label, detail, documentation and insertText fields of the
+// CompletionItem.

rwols wrote:
> ilya-biryukov wrote:
> > Maybe split writes into `Item.label` and other `Item` fields?
> > That would simplify the function, i.e. something like this:
> > 
> > ```
> > // Set Item.label approriately.
> > switch (Result.Kind) {
> >   case RK_Declaration: Item.label = Result.Declaration->getNameAsString();
> >   //... other enum value
> > }
> > 
> > 
> > // Set other Item fields.
> > // Note that is also works as expected for RK_Pattern.
> > auto Completion = Result.CreateCodeCompletionString(/**/);
> > ProcessCodeCompleteString(/*...*/, Item);
> > // 
> > ```
> From some experimentation and skimming over SemaCodeComplete.cpp I notice the 
> result of `CreateCodeCompletionString(/**/)` is never null anyway, so one 
> can just skip those switch cases. I'm not sure why a pointer is returned.
Probably just a way to indicate that memory for returned item is managed by 
`Allocator`.



Comment at: clangd/ClangdUnit.cpp:316
+// CK_Placeholder or CK_CurrentParameter, this will be modified to
+// InsertTextFormat::PlainText.
+Item.insertTextFormat = InsertTextFormat::PlainText;

Probably meant to be `InsertTextFormat::Snippet`, not 
`InsertTextFormat::PlainText`.



Comment at: clangd/ClangdUnit.cpp:379
+
+// TODO: Maybe add an option to allow presenting the optional chunks?
+break;

Style guide: use FIXME instead of TODO



Comment at: clangd/ClangdUnit.cpp:393
+// Terrible hack.
+// TODO: See SemaCodeComplete.cpp:2598
+if (Chunk.Text[0] == ' ')

Style guide: use FIXME instead of TODO



Comment at: clangd/ClangdUnit.cpp:393
+// Terrible hack.
+// TODO: See SemaCodeComplete.cpp:2598
+if (Chunk.Text[0] == ' ')

ilya-biryukov wrote:
> Style guide: use FIXME instead of TODO
Maybe reference function name instead of the line number? Line numbers change 
too often.



Comment at: clangd/ClangdUnit.cpp:395
+if (Chunk.Text[0] == ' ')
+  Item.documentation += "[" + std::string(Chunk.Text + 1) + "] ";
+else

After looking at code completion code, it seems that informative chunks are 
parts of function signature (that have custom highlightings in XCode, probably).
Maybe we could put them into label? It feels that `const` belongs to the 
parameter list and we put parameter list into `label` now.
That would also make the `Chunk.Text + 1` part unnecessary.



Comment at: clangd/ClangdUnit.cpp:452
+
+  void AddSnippetParameter(const char *Text, unsigned ,
+   CompletionItem ) const {

Maybe use `StringRef` instead of `const char*`?



Comment at: clangd/ClangdUnit.cpp:461
+
+  std::string SnippetEscape(const char *Text) const {
+std::string Result;

Maybe use `StringRef` instead of `const char*`?


https://reviews.llvm.org/D37101



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


[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-08-28 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment.

In https://reviews.llvm.org/D37101#853509, @rwols wrote:

> After digging into VSCode, I now think that presenting snippets is the wrong 
> way forward, and I believe the right way is to implement the 
> `signatureHelpProvider` protocol for method/function parameters. See: 
> https://github.com/Microsoft/vscode-docs/blob/master/docs/extensionAPI/language-support.md#help-with-function-and-method-signatures.
>
> I'll update this diff accordingly.


I still think having support for snippets is useful in completion. Let's not 
have snippets for functions and only present them for `RK_Pattern`s.

Signature help is useful, but it's a separate feature and should also go in as 
a separate commit.
For example, consider the differences between completion and signature help.

  vector vec;
  vec.push_back(/*cursor*/

In that case,

- signature help must show all overloads of push_back, show documentation for 
the first parameter, if any;
- code completion must show global completions.


https://reviews.llvm.org/D37101



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


[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-08-27 Thread Raoul Wols via Phabricator via cfe-commits
rwols added a comment.

After digging into VSCode, I now think that presenting snippets is the wrong 
way forward, and I believe the right way is to implement the 
`signatureHelpProvider` protocol for method/function parameters. See: 
https://github.com/Microsoft/vscode-docs/blob/master/docs/extensionAPI/language-support.md#help-with-function-and-method-signatures.

I'll update this diff accordingly.


https://reviews.llvm.org/D37101



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


[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-08-24 Thread Raoul Wols via Phabricator via cfe-commits
rwols marked 10 inline comments as done.
rwols added inline comments.



Comment at: clangd/ClangdUnit.cpp:321
+
+// Fill in the label, detail, documentation and insertText fields of the
+// CompletionItem.

ilya-biryukov wrote:
> Maybe split writes into `Item.label` and other `Item` fields?
> That would simplify the function, i.e. something like this:
> 
> ```
> // Set Item.label approriately.
> switch (Result.Kind) {
>   case RK_Declaration: Item.label = Result.Declaration->getNameAsString();
>   //... other enum value
> }
> 
> 
> // Set other Item fields.
> // Note that is also works as expected for RK_Pattern.
> auto Completion = Result.CreateCodeCompletionString(/**/);
> ProcessCodeCompleteString(/*...*/, Item);
> // 
> ```
From some experimentation and skimming over SemaCodeComplete.cpp I notice the 
result of `CreateCodeCompletionString(/**/)` is never null anyway, so one 
can just skip those switch cases. I'm not sure why a pointer is returned.



Comment at: clangd/ClangdUnit.cpp:381
+// a declarator or macro.
+Item.label += Chunk.Text;
+Item.insertText += Chunk.Text;

ilya-biryukov wrote:
> We set the label in `ProcessCodeCompleteResult`, why is there a need to 
> append to it here?
> Maybe we should set it here and not touch it in `ProcessCodeCompleteResult` 
> at all?
It's possible that maybe too much is going into the label now, I agree. For 
instance for a function the name of the function as well as the opening 
parenthesis, all of its parameters, and the closing brace go into the label. 
Maybe it's enough to only have the name of the function in the label. On the 
other hand, I don't think this will play nicely with VSCode and SublimeText, 
but this will ultimately depend on the language client plugins.



Comment at: clangd/ClangdUnit.cpp:406
+// but should not be inserted into the buffer.
+Item.documentation += "[" + std::string(Chunk.Text) + "] ";
+break;

ilya-biryukov wrote:
> Let's also add some description to `Item.documentation`. What kind of things 
> go into `CK_Informative` ?
> 
I've seen " const", " volatile" go into CK_Informative (note the prefix space). 
Another major chunk that goes into CK_Informative is the base class name 
suffixed with `::` if the completion is a method of the base class not 
overridden in the current class.



Comment at: clangd/ClangdUnit.cpp:413
+break;
+  case CodeCompletionString::CK_CurrentParameter:
+// A piece of text that describes the parameter that corresponds

ilya-biryukov wrote:
> How is `CK_CurrentParameter` different from `CK_Placeholder`? Maybe handle 
> them both simultaneously and add a comment on why we don't care about their 
> differences?
> I mean something like:
> ```
> // We don't distinguish between CK_Placeholder and CK_CurrentParameter 
> because 
> case CodeCompletionString::CK_Placeholder:
> case CodeCompletionString::CK_CurrentParameter:
> //... Code adding ${N:something}
> 
> ```
I'm still figuring out exactly what the difference is...



Comment at: clangd/ClangdUnit.cpp:452
+Item.insertText += Chunk.Text;
+Item.label += Chunk.Text;
+break;

ilya-biryukov wrote:
> Does adding vertical space to `label` plays well with editors? Maybe we 
> should replace with horizontal space instead?
I'll fix this later.


https://reviews.llvm.org/D37101



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


[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-08-24 Thread Raoul Wols via Phabricator via cfe-commits
rwols updated this revision to Diff 112609.
rwols added a comment.

[clangd] [WIP] Add support for snippet completions

- Restore the sortText logic
- Return CompletionItem instead of modifying a ref param
- Make all helper methods const
- Only set to Snippet once we encounter CK_Placeholder/CK_CurrentParameter
- More clear annotation handling
- Escape any $, } and \ for a snippet


https://reviews.llvm.org/D37101

Files:
  clangd/ClangdUnit.cpp
  clangd/Protocol.cpp

Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -724,8 +724,8 @@
   if (!CI.insertText.empty())
 Os << R"("insertText":")" << llvm::yaml::escape(CI.insertText) << R"(",)";
   if (CI.insertTextFormat != InsertTextFormat::Missing) {
-Os << R"("insertTextFormat":")" << static_cast(CI.insertTextFormat)
-   << R"(",)";
+Os << R"("insertTextFormat":)" << static_cast(CI.insertTextFormat)
+   << R"(,)";
   }
   if (CI.textEdit)
 Os << R"("textEdit":)" << TextEdit::unparse(*CI.textEdit) << ',';
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -288,45 +288,189 @@
   void ProcessCodeCompleteResults(Sema , CodeCompletionContext Context,
   CodeCompletionResult *Results,
   unsigned NumResults) override {
-for (unsigned I = 0; I != NumResults; ++I) {
-  CodeCompletionResult  = Results[I];
-  CodeCompletionString *CCS = Result.CreateCodeCompletionString(
+assert(Items && "We need a non-null items container here");
+Items->reserve(NumResults);
+for (unsigned I = 0; I < NumResults; ++I) {
+  auto  = Results[I];
+  const auto *CCS = Result.CreateCodeCompletionString(
   S, Context, *Allocator, CCTUInfo,
   CodeCompleteOpts.IncludeBriefComments);
-  if (CCS) {
-CompletionItem Item;
-for (CodeCompletionString::Chunk C : *CCS) {
-  switch (C.Kind) {
-  case CodeCompletionString::CK_ResultType:
-Item.detail = C.Text;
-break;
-  case CodeCompletionString::CK_Optional:
-break;
-  default:
-Item.label += C.Text;
-break;
-  }
-}
-assert(CCS->getTypedText());
-Item.kind = getKind(Result.CursorKind);
-// Priority is a 16-bit integer, hence at most 5 digits.
-assert(CCS->getPriority() < 9 && "Expecting code completion result "
- "priority to have at most "
- "5-digits");
-llvm::raw_string_ostream(Item.sortText)
-<< llvm::format("%05d%s", CCS->getPriority(), CCS->getTypedText());
-Item.insertText = Item.filterText = CCS->getTypedText();
-if (CCS->getBriefComment())
-  Item.documentation = CCS->getBriefComment();
-Items->push_back(std::move(Item));
-  }
+  assert(CCS && "Expected the CodeCompletionString to be non-null");
+  Items->push_back(ProcessCodeCompleteResult(Result, *CCS));
 }
   }
 
   GlobalCodeCompletionAllocator () override { return *Allocator; }
 
   CodeCompletionTUInfo () override { return CCTUInfo; }
-};
+
+private:
+  CompletionItem
+  ProcessCodeCompleteResult(const CodeCompletionResult ,
+const CodeCompletionString ) const {
+// The object that we'll return.
+CompletionItem Item;
+
+// By default InsertTextFormat::PlainText. When we encounter a
+// CK_Placeholder or CK_CurrentParameter, this will be modified to
+// InsertTextFormat::PlainText.
+Item.insertTextFormat = InsertTextFormat::PlainText;
+
+// Things like __attribute__((nonnull(1,3))) and [[noreturn]]. Present this
+// information in the documentation field.
+if (CCS.getAnnotationCount() > 0) {
+  Item.documentation += "Annotation";
+  if (CCS.getAnnotationCount() == 1) {
+Item.documentation += ": ";
+  } else /* CCS.getAnnotationCount() > 1 */ {
+Item.documentation += "s: ";
+  }
+  for (unsigned j = 0; j < CCS.getAnnotationCount(); ++j) {
+Item.documentation += CCS.getAnnotation(j);
+Item.documentation += j == CCS.getAnnotationCount() - 1 ? "\n\n" : " ";
+  }
+}
+
+// Fill in the label, detail, documentation and insertText fields of the
+// CompletionItem. The informative chunks are put into the documentation
+// field.
+ProcessChunks(CCS, Item);
+
+// Add brief documentation (if there is any).
+if (CCS.getBriefComment() != nullptr) {
+  Item.documentation += CCS.getBriefComment();
+}
+
+// Fill in the kind field of the CompletionItem.
+Item.kind = getKind(Result.CursorKind);
+
+// Fill in the sortText of the CompletionItem.
+assert(CCS.getPriority() < 9 && "Expecting code 

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-08-24 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment.

In https://reviews.llvm.org/D37101#851380, @rwols wrote:

> Thanks for the quick review! I'm new to Phabricator and the `arc` CLI tool. 
> Is the workflow like this: "address a comment, change a few lines, do `arc 
> diff`, do this multiple times", or is it like this: "address all the 
> comments, change lots of lines, do `arc diff`, do this once"?


Both workflows work,  but I would suggest to address a batch of comments before 
doing `arc diff`, as it may become hard to follow inline comments after 
multiple `arc diff`s.
You can also send multiple commits in a single `arc diff` if you use `git-svn`. 
I highly suggest looking into that workflow if you're familiar with `git`. 
There's also an `arc patch` that you can use to apply a patch from this review 
after doing a new `git-svn` checkout.


https://reviews.llvm.org/D37101



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


[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-08-24 Thread Raoul Wols via Phabricator via cfe-commits
rwols added a comment.

Thanks for the quick review! I'm new to Phabricator and the `arc` CLI tool. Is 
the workflow like this: "address a comment, change a few lines, do `arc diff`, 
do this multiple times", or is it like this: "address all the comments, change 
lots of lines, do `arc diff`, do this once"?


https://reviews.llvm.org/D37101



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


[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-08-24 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.

Thanks for taking your time to implement this!

We need to add some test cases for new completion features. However, adding 
them may be a bit of a pain, so feel free to ask for best practices if you'll 
run into trouble while doing that.
Please also see my inline comments.




Comment at: clangd/ClangdUnit.cpp:293
+// earlier in the sequence.
+std::stable_sort(Results, Results + NumResults,
+ [](const CodeCompletionResult ,

Is this required for snippets?
I suggest we move the removal of `sortText` into a separate commit/discussion.



Comment at: clangd/ClangdUnit.cpp:295
+ [](const CodeCompletionResult ,
+const CodeCompletionResult ) -> bool {
+   return lhs.Priority > rhs.Priority;

Code style: use `UpperCamelCase` for parameter names



Comment at: clangd/ClangdUnit.cpp:299
 for (unsigned I = 0; I != NumResults; ++I) {
-  CodeCompletionResult  = Results[I];
-  CodeCompletionString *CCS = Result.CreateCodeCompletionString(
-  S, Context, *Allocator, CCTUInfo,
-  CodeCompleteOpts.IncludeBriefComments);
-  if (CCS) {
+  if (Results[I].Availability == CXAvailability_NotAvailable ||
+  Results[I].Availability == CXAvailability_NotAccessible) {

Could you please also move this to a separate review?
I think we should show inaccessible items, but they should have a much lower 
priority than accessible ones. Others may disagree, but let's have this 
discussion in a separate thread.



Comment at: clangd/ClangdUnit.cpp:305
 CompletionItem Item;
-for (CodeCompletionString::Chunk C : *CCS) {
-  switch (C.Kind) {
-  case CodeCompletionString::CK_ResultType:
-Item.detail = C.Text;
-break;
-  case CodeCompletionString::CK_Optional:
-break;
-  default:
-Item.label += C.Text;
-break;
-  }
-}
-assert(CCS->getTypedText());
-Item.kind = getKind(Result.CursorKind);
-// Priority is a 16-bit integer, hence at most 5 digits.
-assert(CCS->getPriority() < 9 && "Expecting code completion result 
"
- "priority to have at most "
- "5-digits");
-llvm::raw_string_ostream(Item.sortText)
-<< llvm::format("%05d%s", CCS->getPriority(), CCS->getTypedText());
-Item.insertText = Item.filterText = CCS->getTypedText();
-if (CCS->getBriefComment())
-  Item.documentation = CCS->getBriefComment();
+this->ProcessCodeCompleteResult(S, Context, Results[I], Item);
 Items->push_back(std::move(Item));

Code style: we usually don't use explicit `this->` qualifiers.



Comment at: clangd/ClangdUnit.cpp:316
+private:
+  void ProcessCodeCompleteResult(Sema , CodeCompletionContext Context,
+ CodeCompletionResult ,

Maybe return `CompletionItem` instead of modifying a reference parameter?



Comment at: clangd/ClangdUnit.cpp:316
+private:
+  void ProcessCodeCompleteResult(Sema , CodeCompletionContext Context,
+ CodeCompletionResult ,

ilya-biryukov wrote:
> Maybe return `CompletionItem` instead of modifying a reference parameter?
Any reason why `Context` is passed by value?  Maybe pass by `const &`?



Comment at: clangd/ClangdUnit.cpp:317
+  void ProcessCodeCompleteResult(Sema , CodeCompletionContext Context,
+ CodeCompletionResult ,
+ CompletionItem ) {

This function does not modify `Result`. Maybe pass by `const &` instead of `&`?



Comment at: clangd/ClangdUnit.cpp:321
+
+// Fill in the label, detail, documentation and insertText fields of the
+// CompletionItem.

Maybe split writes into `Item.label` and other `Item` fields?
That would simplify the function, i.e. something like this:

```
// Set Item.label approriately.
switch (Result.Kind) {
  case RK_Declaration: Item.label = Result.Declaration->getNameAsString();
  //... other enum value
}


// Set other Item fields.
// Note that is also works as expected for RK_Pattern.
auto Completion = Result.CreateCodeCompletionString(/**/);
ProcessCodeCompleteString(/*...*/, Item);
// 
```



Comment at: clangd/ClangdUnit.cpp:362
+// Always a snippet for now.
+Item.insertTextFormat = InsertTextFormat::Snippet;
+  }

Maybe only set it to `Snippet` if we encountered `CK_Placeholder`?
That would allow clients of 

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

2017-08-24 Thread Raoul Wols via Phabricator via cfe-commits
rwols created this revision.

Enhances CompletionItemsCollector in such a way that snippet
completions are presented to the client.
This is a work-in-progress. It currently works in Sublime Text 3 using the new
"LSP" plugin. In VSCode, the snippets are inserted into the buffer as plaintext
without any processing (bug). Untested in Atom.
See: 
https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#completion-request
See: 
https://github.com/Microsoft/vscode/blob/master/src/vs/editor/contrib/snippet/browser/snippet.md


https://reviews.llvm.org/D37101

Files:
  clangd/ClangdUnit.cpp
  clangd/Protocol.cpp

Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -724,8 +724,8 @@
   if (!CI.insertText.empty())
 Os << R"("insertText":")" << llvm::yaml::escape(CI.insertText) << R"(",)";
   if (CI.insertTextFormat != InsertTextFormat::Missing) {
-Os << R"("insertTextFormat":")" << static_cast(CI.insertTextFormat)
-   << R"(",)";
+Os << R"("insertTextFormat":)" << static_cast(CI.insertTextFormat)
+   << R"(,)";
   }
   if (CI.textEdit)
 Os << R"("textEdit":)" << TextEdit::unparse(*CI.textEdit) << ',';
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -288,45 +288,180 @@
   void ProcessCodeCompleteResults(Sema , CodeCompletionContext Context,
   CodeCompletionResult *Results,
   unsigned NumResults) override {
+// Sort the completions by their priority. A higher priority should come
+// earlier in the sequence.
+std::stable_sort(Results, Results + NumResults,
+ [](const CodeCompletionResult ,
+const CodeCompletionResult ) -> bool {
+   return lhs.Priority > rhs.Priority;
+ });
 for (unsigned I = 0; I != NumResults; ++I) {
-  CodeCompletionResult  = Results[I];
-  CodeCompletionString *CCS = Result.CreateCodeCompletionString(
-  S, Context, *Allocator, CCTUInfo,
-  CodeCompleteOpts.IncludeBriefComments);
-  if (CCS) {
+  if (Results[I].Availability == CXAvailability_NotAvailable ||
+  Results[I].Availability == CXAvailability_NotAccessible) {
+// A private member variable outside of the class, for example.
+continue;
+  } else {
 CompletionItem Item;
-for (CodeCompletionString::Chunk C : *CCS) {
-  switch (C.Kind) {
-  case CodeCompletionString::CK_ResultType:
-Item.detail = C.Text;
-break;
-  case CodeCompletionString::CK_Optional:
-break;
-  default:
-Item.label += C.Text;
-break;
-  }
-}
-assert(CCS->getTypedText());
-Item.kind = getKind(Result.CursorKind);
-// Priority is a 16-bit integer, hence at most 5 digits.
-assert(CCS->getPriority() < 9 && "Expecting code completion result "
- "priority to have at most "
- "5-digits");
-llvm::raw_string_ostream(Item.sortText)
-<< llvm::format("%05d%s", CCS->getPriority(), CCS->getTypedText());
-Item.insertText = Item.filterText = CCS->getTypedText();
-if (CCS->getBriefComment())
-  Item.documentation = CCS->getBriefComment();
+this->ProcessCodeCompleteResult(S, Context, Results[I], Item);
 Items->push_back(std::move(Item));
   }
 }
   }
 
   GlobalCodeCompletionAllocator () override { return *Allocator; }
 
   CodeCompletionTUInfo () override { return CCTUInfo; }
-};
+
+private:
+  void ProcessCodeCompleteResult(Sema , CodeCompletionContext Context,
+ CodeCompletionResult ,
+ CompletionItem ) {
+unsigned ArgCount = 0;
+
+// Fill in the label, detail, documentation and insertText fields of the
+// CompletionItem.
+switch (Result.Kind) {
+case CodeCompletionResult::RK_Declaration: {
+  auto Completion = Result.CreateCodeCompletionString(
+  S, Context, *Allocator, CCTUInfo,
+  CodeCompleteOpts.IncludeBriefComments);
+  if (!Completion) {
+Item.label = Result.Declaration->getNameAsString();
+  } else {
+ProcessCodeCompleteString(*Completion, ArgCount, Item);
+  }
+  break;
+}
+case CodeCompletionResult::RK_Keyword:
+  Item.label = Result.Keyword;
+  break;
+case CodeCompletionResult::RK_Macro: {
+  auto Completion = Result.CreateCodeCompletionString(
+  S, Context, *Allocator, CCTUInfo,
+  CodeCompleteOpts.IncludeBriefComments);
+  if (!Completion) {
+Item.label = Result.Macro->getName().str();
+  } else {
+