This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ioeric on Dec 20 2017, 7:14 AM.

Diff Detail

Event Timeline

ioeric created this revision.Dec 20 2017, 7:14 AM
sammccall accepted this revision.Dec 20 2017, 8:27 AM
sammccall added inline comments.
clangd/CompletionString.cpp
51 ↗(On Diff #127718)

if you actually care about performance, escapeSnippet should take the string to append to as a parameter

67 ↗(On Diff #127718)

these comments are just copies of the enum documentation, mind removing them while here?

118 ↗(On Diff #127718)

you've grouped these into a default case above - do the same here?

clangd/CompletionString.h
1 ↗(On Diff #127718)

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

25 ↗(On Diff #127718)

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.

28 ↗(On Diff #127718)

nit: std::pair is always hard to remember - can we take two out-params so code-completion can help us?

34 ↗(On Diff #127718)

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.

37 ↗(On Diff #127718)

nit: result -> return type

unittests/clangd/CompletionStringTests.cpp
89 ↗(On Diff #127718)

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

This revision is now accepted and ready to land.Dec 20 2017, 8:27 AM
This revision was automatically updated to reflect the committed changes.
ioeric marked 7 inline comments as done.