Page MenuHomePhabricator

[CodeComplete] Tweak completion for else.
ClosedPublic

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

Details

Summary

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

Unit TestsFailed

TimeTest
9,950 mslinux > libomp.env::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/env/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && env KMP_DISP_NUM_BUFFERS=0 /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp
1,340 mslinux > libomp.worksharing/for::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/worksharing/for/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp 7

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
}

Suggestions:

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

With:

if (...) 
  // Statement
}

Suggestions:

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

clang/lib/Sema/SemaCodeComplete.cpp
5780

name could be clearer here: AddElseBodyPattern?

5789

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.

5791

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)

5795

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.