Page MenuHomePhabricator

clang-format: support aligned nested conditionals formatting
Needs ReviewPublic

Authored by Typz on Jul 31 2018, 9:31 AM.

Details

Summary

When multiple ternary operators are chained, e.g. like an if/else-if/
else-if/.../else sequence, clang-format will keep aligning the colon
with the question mark, which increases the indent for each
conditionals:

int a = condition1 ? result1
                   : condition2 ? result2
                                : condition3 ? result3
                                             : result4;

This patch detects the situation (e.g. conditionals used in false branch
of another conditional), to avoid indenting in that case:

int a = condition1 ? result1
      : condition2 ? result2
      : condition3 ? result3
                   : result4;

When BreakBeforeTernaryOperators is false, this will format like this:

int a = condition1 ? result1 :
        condition2 ? result2 :
        conditino3 ? result3 :
                     result4;

This formatting style is referenced here:
https://www.fluentcpp.com/2018/02/27/replace-else-if-ternary-operator/
and here:
https://marcmutz.wordpress.com/2010/10/14/top-5-reasons-you-should-love-your-ternary-operator/

Diff Detail

Event Timeline

Typz created this revision.Jul 31 2018, 9:31 AM
Typz added a comment.Jul 31 2018, 9:48 AM

Notes:

  • I choose not to add an option to enable this behavior, as I think of it as just another way clang-format can (better) format the code; but I can one if needed
  • Currently, it relies on another patch (D32478), which supports aligning the wrapped operator slightly differently. If/since that other patch does not seem to make it, I can change this patch to either do the alignment in this specific case (e.g. for wrapped ternary operator only) or to keep the 'default' behavior of clang-format (e.g. the wrapped colon would be aligned with the first condition):
// i.e. the better looking option, but which requires specific cases when it comes after assignment or return:
int fooo = aaaa ? 00000
         : bbbb ? 11111
                : 2222;
// or the option more consistent with current clang-format behavior:
int fooo = aaaa   ? 00000
           : bbbb ? 11111
                  : 2222;

The current patch supports both behaviors, as it relies on D32478 to specify the expected behavior.

I don't have very strong opinions here and I agree that we will need to improve the conditional formatting, especially as this kind of ternary-chaining is becoming more popular because of constexpr.

My personal opinion(s):

  • I think this is a no-brainer for BreakBeforeTernaryOperators=false
  • I'd be ok with the suggestion for BreakBeforeTernaryOperators=true
  • I don't think the alignment of "?" and ":" (in the WhitespaceManager) should be always on. I think we'd need a flag for that part

Manuel, Krasimir, WDYT?

klimek added a comment.Aug 1 2018, 3:03 AM

Yea, if we go down this route I'd go with this by default:

some ? thing :
else ? otherthing :
unrelated ? but :
    finally;

Theoretically we could even use:

some ? thing :
    else;

by default ;)

Could you clarify how each piece is supposed to be aligned in these examples?
This is what makes me happy:

// column  limit             V
int a = condition1 ? result1
      : conditio2 ? result2
      : loooooooooocondition 
        ? result2
        : dition3 ? resul3
                  : resul4;
// column  limit             V
int a = condition1 
        ? loooooresult1
        : conditio2 ? result2
                    : result4;

When BreakBeforeTernaryOperators is false:

int a = condition1 ? result1 :
        conditio2 ? result2 :
        ditino3 ? resul3 :
                  result4;
Typz added a comment.Aug 1 2018, 6:56 AM
  • I'd be ok with the suggestion for BreakBeforeTernaryOperators=true

Just to be clear, which suggestion would you be ok with?

int fooo = aaaa ? 00000
         : bbbb ? 11111
                : 2222;

or:

int fooo = aaaa   ? 00000
           : bbbb ? 11111
                  : 2222;
  • I don't think the alignment of "?" and ":" (in the WhitespaceManager) should be always on. I think we'd need a flag for that part

No problem, I'll add the option.

Typz added a comment.Aug 1 2018, 7:22 AM

Could you clarify how each piece is supposed to be aligned in these examples?
This is what makes me happy:

// column  limit             V
int a = condition1 ? result1
      : conditio2 ? result2
      : loooooooooocondition 
        ? result2
        : dition3 ? resul3
                  : resul4;
// column  limit             V
int a = condition1 
        ? loooooresult1
        : conditio2 ? result2
                    : result4;

It gives the following:

// column  limit             V
int a = condition1 ? result1
      : conditio2 ? result2
      : loooooooooocondition
          ? result2
      : dition3 ? resul3
                : resul4;

// column  limit             V
int a = condition1
          ? loooooresult1
      : conditio2 ? result2
                  : result4;

i.e. the long result is wrapped and gets an extra indentation.
I have tried quite a bit to "fall back" to the old behavior when there is this kind of wrapping, but this always created other situations which got brocken because of this: so finally I choose to stay consistent, and apply the same behavior whenever there are chained conditionals.

When BreakBeforeTernaryOperators is false:

int a = condition1 ? result1 :
        conditio2 ? result2 :
        ditino3 ? resul3 :
                  result4;

This ones is indeed aligned like this.

Herald added a project: Restricted Project. · View Herald TranscriptWed, May 22, 9:35 AM