Page MenuHomePhabricator

[CodeComplete] Tweak completion for else.

Authored by njames93 on Jun 26 2020, 12:51 AM.



If an if statement uses braces for its then block, suggest braces for the else and else if completion blocks, Otherwise don't suggest them.

Diff Detail

Event Timeline

njames93 created this revision.Jun 26 2020, 12:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2020, 12:51 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I'm still unsure what is the best behaviour here.
Would suggesting both patterns, but sort them based on what the then branch uses be best
Example with:

if (...) {
  // Statements


- else { // Statements }
- else if (...) { // Statements }
- else // Statement
- else if (...) // Statement


if (...) 
  // Statement


- else // Statement
- else if (...) // Statement
- else { // Statements }
- else if (...) { // Statements }

Or is it best keeping with the behaviour of this patch where only 1 suggestion appears based on whether braces are used or not??

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.


name could be clearer here: AddElseBodyPattern?


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.


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)


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)

njames93 updated this revision to Diff 274469.Jun 30 2020, 7:22 AM

Address comments and add tests.

njames93 marked 4 inline comments as done.Jun 30 2020, 7:25 AM
sammccall accepted this revision.Jun 30 2020, 8:38 AM

Nice, thanks for doing this!

This revision is now accepted and ready to land.Jun 30 2020, 8:38 AM
This revision was automatically updated to reflect the committed changes.