Details
Diff Detail
- Repository
- rCTE Clang Tools Extra
- Build Status
Buildable 13332 Build 13332: arc lint + arc unit
Event Timeline
clangd/CompletionString.cpp | ||
---|---|---|
51 | if you actually care about performance, escapeSnippet should take the string to append to as a parameter | |
67 | these comments are just copies of the enum documentation, mind removing them while here? | |
118 | you've grouped these into a default case above - do the same here? | |
clangd/CompletionString.h | ||
1 | nit: CodeCompletionStrings? plural because it doesn't define the type, but operations on it. | |
25 | 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 | nit: std::pair is always hard to remember - can we take two out-params so code-completion can help us? | |
34 | 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 | nit: result -> return type | |
unittests/clangd/CompletionStringTests.cpp | ||
89 | 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... |
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...