Page MenuHomePhabricator

[clang-format] Introduce NoFallThrough option into AllowShortCaseLabelsOnASingleLine
AcceptedPublic

Authored by MyDeveloperDay on Sep 9 2022, 5:39 AM.

Details

Summary

As highlighted by https://github.com/llvm/llvm-project/issues/57609

Give more control when using the AllowShortCaseLabelsOnASingleLine to not put fallthrough code onto the same line

switch (i) {
case 0: g(); [[fallthrough]];
case 1: f(); break;
}

whether that fallthrough is marked with the attribute or implicit

switch (i) {
case 0:
case 1: return i;
case 2:
case 3: i *= 2; break;
}

in these cases don't make them short

switch (i) {
case 0:
  g();
  [[fallthrough]];
case 1:
  f();
  break;
}
switch (i) {
case 0:
case 1:
  return i;
case 2:
case 3:
  i *= 2;
  break;
}

Fixes https://github.com/llvm/llvm-project/issues/57609

Diff Detail

Event Timeline

MyDeveloperDay created this revision.Sep 9 2022, 5:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2022, 5:39 AM
MyDeveloperDay requested review of this revision.Sep 9 2022, 5:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2022, 5:39 AM
curdeius edited the summary of this revision. (Show Details)Sep 9 2022, 5:55 AM
curdeius added inline comments.Sep 9 2022, 6:04 AM
clang/docs/ClangFormatStyleOptions.rst
935
3596

Unrelated change.

clang/lib/Format/UnwrappedLineFormatter.cpp
643–657

Can we merge the conditions like this?

MyDeveloperDay marked 3 inline comments as done.
  • rebase with fixed doc NFC update
  • rework the nofallback condition to prevent repeated checks
  • fix grammar checks
curdeius accepted this revision.Sep 9 2022, 1:25 PM

LGTM after fixing the last comment.

clang/lib/Format/UnwrappedLineFormatter.cpp
650

Haven't seen this before.

This revision is now accepted and ready to land.Sep 9 2022, 1:25 PM
owenpan added inline comments.Sep 9 2022, 11:25 PM
clang/lib/Format/UnwrappedLineFormatter.cpp
567

See below.

635–636

We don't need to add Style.

643–644

Nit.

646

Last should never be null.

650–653

Use endsSequence instead.

clang/include/clang/Format/Format.h
469

While we're at it, shouldn't there be a Leave? ;)

MyDeveloperDay marked an inline comment as done.Sep 12 2022, 12:14 AM
MyDeveloperDay added inline comments.
clang/include/clang/Format/Format.h
469

I think Leave is Never

clang/include/clang/Format/Format.h
469

No. Leave would leave me with this:

switch (a) {
  case 1: x = 1; break;
  case 2:
    return;
}

If I'd start with it. Leave allows to mix the Yes and No options.