sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:35
+void appendCodeCompletionString(const CodeCompletionString &CCS, std::string
*Out) {
+ for (const CodeCompletionString::Chunk &C : CCS) {
----------------
maybe appendOptionalChunk? if in doubt, better to describe the semantics of the
param than its type, since the type appears in the signature
================
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:156
case CodeCompletionString::CK_Optional:
+ assert(Chunk.Optional &&
+ "Expected the optional code completion string to be non-null.");
----------------
please drop the extra message from the assert unless there's something
additional to say (it just echoes the expression here)
(I don't have a strong opinion on this assert - I'm fine with keeping it but
you could also drop it as we're enforcing someone else's clearly documented
invariant)
================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:943
+
+TEST(CompletionTest, DefaultArgs) {
+ clangd::CodeCompleteOptions Opts;
----------------
for completeness, could you also add a unit test to
CodeCompletionStringsTests.cpp?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65866/new/
https://reviews.llvm.org/D65866
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
