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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
