One-line case being split up

[jfbastien]

Bugzilla Link	16535
Version	trunk
OS	All
CC	@chandlerc,@silvasean

Extended Description

Your suggested formatting:

int foo(int i) {
  switch (i) {
  default: return 5;
  case 0: return 3;
  case 1: return 2;
  case 2: return 1;
  case 3: return 0;
  }
}

clang-format's current output:

int foo(int i) {
  switch (i) {
  default:
    return 5;
  case 0:
    return 3;
  case 1:
    return 2;
  case 2:
    return 1;
  case 3:
    return 0;
  }
}

Additional information:
It's nice to keep the code concise if it originally was.

[chandlerc]

I'm actually very OK if we want to teach clang-format to do really nice column-layout of switches where that fits.

As an initial suggestion, I would restrict it to the following rules:

1. Can form 2 or 3 aligned columns for every case without exceeding the column limit.
2. Allow the default entry to be laid out in a non-column format if it doesn't fit in 80 columns.
3. First column always contains the case.
   4a) Second column contains exactly a return statement in a 2-column layout.
   4b) Second column contains exactly one statement in a 3-column layout, the statement must not be a break, return, or other control flow statement.
4. Third column (if it exists ) must be consistent for every case and be either a break statement or a return statement.

Thoughts?

[jfbastien]

SGTM in general, though:

• Having one line cases versus nicely aligning them is a bit different.
• How about allowing some multi-line cases, and some clusters of single-line ones?
• What about intervening comments? Either at end of line, or between cases?
• What about fallthrough cases?

These can be tackled independently, of course, and what you propose can be a starting point. Here are some examples.

  // Column alignment, with one weird long case:
  switch (i) {
  default: op = MyNamespace::Default;   break;
  case 0:  op = MyNamespace::ZeroValue; break;
  case 1:  op = MyNamespace::OneValue;  break;
  case 2:
    op = MyNamespace::TwoValueIsAReallyLongThingThatBreaksThePattern;
    break;
  case 3:  op = MyNamespace::ThreeValue; break;
  }

  // Comments:
  switch (i) {
  default: return 5; // I guess you wanted 5?
  case 0: return 3;
  case 1: return 2;
  // This one is a funny case.
  case 2: return 1;
  case 3: return 0;
  }

  // Fallthrough:
  switch (i) {
  default: return 5;
  case 0: return 3;
  case 1:
  case 2: return 1;
  case 3: return 0;
  }

[silvasean]

Just ran into this in some code I'm writing. For large switches doubling the number of lines can really really hurt readability. I think that nice alignment would be great, but is a separate issue.

[llvmbot]

This has been partly addressed by r217501. There now is a new option to allow putting short case labels on a single line. I have not activated it by default for any style as I don't know whether it is always desirable (especially until we are also able to lay these out in columns).

[llvmbot]

I'd really like to change AllowShortCaseLabelsOnASingleLine to true for my project but as currently implemented, fall-through (and maybe comments) aren't handled well. I'd prefer fall-through to revert the case that finally handles it to multi-line. Example:

// What clang-format currently does
switch (nodeIteratorType) {
  case NodeIteratorType::Empty:
  case NodeIteratorType::Vector: return vector_case();
  case NodeIteratorType::Map: return map_case();
  default: throw std::out_of_range("Invalid NodeIteratorType");
}

// What I'd like it to do
switch (nodeIteratorType) {
  case NodeIteratorType::Empty:
  case NodeIteratorType::Vector:
    return vector_case();
  case NodeIteratorType::Map: return map_case();
  // Other one-line cases here...
  default: throw std::out_of_range("Invalid NodeIteratorType");
}

This bug hasn't seen much action for its ~1.5 years of existence but I do see its been recently reassigned. Is this something that may actually happen?

Thanks.

[llvmbot]

Not likely unless a new contributor turns up.

[llvmbot]

Good that we have someone, then :-).

[llvmbot]

Argh... Wrong bug. Ignore the previous comment.

[llvmbot]

mentioned in issue llvm/llvm-bugzilla-archive#19067

[HazardyKnusperkeks]

Can be achieved by AllowShortCaseLabelsOnASingleLine: true.