[PATCH] D41450: [clangd] Pull CodeCompletionString handling logic into its own file and add unit test.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clangd/CompletionString.cpp:51
+  std::string Result;
+  Result.reserve(Text.size()); // Assume '$', '}' and '\\' are rare.
+  for (const auto Character : Text) {
----------------
if you actually care about performance, escapeSnippet should take the string to append to as a parameter


================
Comment at: clangd/CompletionString.cpp:67
+  for (const auto &Chunk : CCS) {
+    // Informative qualifier chunks only clutter completion results, skip
+    // them.
----------------
these comments are just copies of the enum documentation, mind removing them while here?


================
Comment at: clangd/CompletionString.cpp:118
+      break;
+    case CodeCompletionString::CK_LeftParen:
+      // A left parenthesis ('(').
----------------
you've grouped these into a default case above - do the same here?


================
Comment at: clangd/CompletionString.h:1
+//===--- CompletionString.h --------------------------------------*- C++-*-===//
+//
----------------
nit: `CodeCompletionStrings`?

plural because it doesn't define the type, but operations on it.
Full name because we have too many ways to spell things already...


================
Comment at: clangd/CompletionString.h:25
+///
+/// If \p IsSnippet is not nullptr, this will try to use snippet for the insert
+/// text and sets `IsSnippet` to true when a snippet is created. Otherwise, the
----------------
This IsSnippet signature is clever, and matches the existing behavior (even if snippets are supported, we send plaintext if possible).

However that doesn't seem important to preserve - either snippets are supported or they're not, and the logic is simpler if we just tell getLabelAndInsertText what style we want.


================
Comment at: clangd/CompletionString.h:28
+/// insert text will always be plain text.
+std::pair<std::string, std::string>
+getLabelAndInsertText(const CodeCompletionString &CCS,
----------------
nit: std::pair is always hard to remember - can we take two out-params so code-completion can help us?


================
Comment at: clangd/CompletionString.h:34
+/// a class declaration.
+std::string getDocumentation(const CodeCompletionString &CCS);
+
----------------
I'm skeptical that CodeCompletionString is actually where we want to be generating documentation in the long run, vs something like Decl which will give us more flexibility. But this matches what we currently do and seems fine for now.


================
Comment at: clangd/CompletionString.h:37
+/// Gets detail to be used as the detail field in an LSP completion item. This
+/// is usually the result of a function.
+std::string getDetail(const CodeCompletionString &CCS);
----------------
nit: result -> return type


================
Comment at: unittests/clangd/CompletionStringTests.cpp:89
+
+TEST_F(CompletionStringTest, FunctionSnippet) {
+  Builder.AddResultTypeChunk("result no no");
----------------
I like the fine-grained tests of the features, but I'd also like to be able to see how these strings compare for more typical examples, like the one in this test.

Could you have a test (maybe this one or maybe a new one) where you build a typical function CCS, and then assert all the strings: label, insert text, filter text, with and without snippets...


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41450