[PATCH] D82626: [CodeComplete] Tweak completion for else.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I like the behavior in this patch - I think multiple options is going to be distracting and crowd out other good results, and the heuristic seems pretty good.

This needs a test in e.g. `llvm-project/clang/test/CodeCompletion/patterns.cpp`.



================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5764
+
+  auto AddCodePatterns = [&] {
+    if (IsBracedThen) {
----------------
name could be clearer here: AddElseBodyPattern?


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5773
+    } else {
+      // HorizontalSpace so the hint looks like 'else statement' instead of
+      // 'elsestatement'.
----------------
Hmm, this is in clangd I guess, which ignores vertical space when rendering snippets.
Maybe that's a bug there, and clangd should include vertical space as space, but collapse consecutive ones?
I wonder what other consumers do.

If we do keep it (also fine) you could switch the order of the vertical and horizontal space, which suggests indentation. Up to you though.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5775
+      // 'elsestatement'.
+      // FIXME, would be nicer if there was a CK_IndentationSpace option that
+      // would add the correct number of tabs/spaces.
----------------
I'm not sure about this FIXME, I think it's pretty unlikely this will happen (this layer of clang is unlikely to retain enough info and know enough about style to do this, and if it's solved at another layer it's likely to be clang-format that doesn't require this hint)


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5779
+      Builder.AddChunk(CodeCompletionString::CK_VerticalSpace);
+      Builder.AddPlaceholderChunk("statement");
+    }
----------------
do you want to insert the semicolon after "statement"? (i.e. the semicolon is not part of the placeholder)
This seems likely to play better with e.g. clang-format, and should give more accurate diagnostics if the user hasn't filled in the placeholders yet.

(I don't see much precedent one way or the other, there's only "statements" in blocks... the for-loop snippet obviously inserts semicolons though)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82626/new/

https://reviews.llvm.org/D82626