clang-format: support aligned nested conditionals formatting

[Typz]

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/

[Typz]

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.

[djasper]

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]

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 ;)

[krasimir]

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]

In D50078#1184015, @djasper wrote:

• 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]

In D50078#1184159, @krasimir wrote:

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.

[Typz]

ping ?

[Typz]

ping ?

[Typz]

ping ?

[Typz]

Rebase

[MyDeveloperDay]

Thank you for this patch, (and by your patience ;-) )

This LGTM, Were there any objections from anyone else? otherwise I'd say this was ok.

[Typz]

This actually depends on another Diff : https://reviews.llvm.org/D32478
That other change introduces the ability to "unindent operator", which is required here; however, it also changes AlignOperands to have more than 2 modes, which does not seem to be acceptable.

Before merge, should I thus merge "unindent operator" ability back in this change? Or create another commit for that part only, to be reviewed separately?

[Typz]

Rebased on master, integrating required code from https://reviews.llvm.org/D32478

[MyDeveloperDay]

You know that feeling when you are doing a review and you think... I'm just not a compiler... I feel given the previous discussion and the level of extra tests maybe this is just worth going ahead with as long as you are prepared to support it in the interim.

clang/unittests/Format/FormatTest.cpp

5918

Nit: I'm just slightly confused as to what is happening here.. I assume this is the case where they are not aligned in the style.

6186

as above how does one get back to the original case?

[Typz]

Fix corner in previous rebase

[MyDeveloperDay]

LGTM

[Typz]

Fix earlier error in patch upload.

[Typz]

Sorry, I struggled a bit with git & phabricator, and ended up uploading non-working code :-/

clang/unittests/Format/FormatTest.cpp

5918

Ternary operator do indeed get aligned, thus :

• the "tests" (on first and third lines) get aligned
• the "results" do not fit on the line, thus get further wrapped

6186

I tried this quite a bit, but could not find a reliable way to "skip" the ternary operator alignment when result operand get wrapped.
If you have an idea/hint how to do this, I'd be glad to give a try...

[Typz]

Implement a fallback to regularly indented alignment when the question mark is wrapped.

[Typz]

clang/lib/Format/ContinuationIndenter.cpp

1169

Maybe it would be better to go through here whenever this is wrapped (e.g. Newline == true) : but this implies akso introducing a penalty for "breaking" the nested conditional alignment. May ultimately be better, though probably somewhat more complex still.

[dyung]

Hi, this change that you submitted in commit 5daa25fd7a184524759b6ad065a8bd7e95aa149a seems to be causing the test "Clang-Unit :: Format/./FormatTests.exe/FormatTest.ConfigurableUseOfTab" to randomly fail about 50% of the time on Windows.

Take the most recent 5 runs of the llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast buildbot. Builds #31973, 31975 and 31976 all failed because of that test failing.

On my local Windows computer, if I build your commit and run the FormatTest unit test multiple times (the same binary without changing anything or rebuilding), that above test fails roughly 50% of the time.

Can you take a look to figure out why your change might be causing this instability?

[Typz]

In D50078#1998398, @dyung wrote:

Hi, this change that you submitted in commit 5daa25fd7a184524759b6ad065a8bd7e95aa149a seems to be causing the test "Clang-Unit :: Format/./FormatTests.exe/FormatTest.ConfigurableUseOfTab" to randomly fail about 50% of the time on Windows.

Take the most recent 5 runs of the llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast buildbot. Builds #31973, 31975 and 31976 all failed because of that test failing.

On my local Windows computer, if I build your commit and run the FormatTest unit test multiple times (the same binary without changing anything or rebuilding), that above test fails roughly 50% of the time.

Can you take a look to figure out why your change might be causing this instability?

Hi,
That is very strange indeed, nothing is supposed to be random here...
I will try to have a look, but I don't have a windows computer to test, and never seen this issue on linux/macos :-/

[Typz]

This comment has been deleted.

clang/lib/Format/WhitespaceManager.h

164

This field is not initialised in constructor of WhitespaceManager::Change class (WhitespaceManager.cpp:43). Maybe this could be the issue.

[hokein]

In D50078#1998520, @Typz wrote:

In D50078#1998398, @dyung wrote:

Hi, this change that you submitted in commit 5daa25fd7a184524759b6ad065a8bd7e95aa149a seems to be causing the test "Clang-Unit :: Format/./FormatTests.exe/FormatTest.ConfigurableUseOfTab" to randomly fail about 50% of the time on Windows.

Take the most recent 5 runs of the llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast buildbot. Builds #31973, 31975 and 31976 all failed because of that test failing.

On my local Windows computer, if I build your commit and run the FormatTest unit test multiple times (the same binary without changing anything or rebuilding), that above test fails roughly 50% of the time.

Can you take a look to figure out why your change might be causing this instability?

Hi,
That is very strange indeed, nothing is supposed to be random here...
I will try to have a look, but I don't have a windows computer to test, and never seen this issue on linux/macos :-/

The linux (s390x arch) buildbot is also failing, http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/31811/steps/ninja%20check%201/logs/stdio.
I reverted it in 47ef09e4848a970c530928496b54085cfdba5a76 to make buildbot happy.

[Typz]

In D50078#1998578, @hokein wrote:

In D50078#1998520, @Typz wrote:

In D50078#1998398, @dyung wrote:

Hi, this change that you submitted in commit 5daa25fd7a184524759b6ad065a8bd7e95aa149a seems to be causing the test "Clang-Unit :: Format/./FormatTests.exe/FormatTest.ConfigurableUseOfTab" to randomly fail about 50% of the time on Windows.

Take the most recent 5 runs of the llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast buildbot. Builds #31973, 31975 and 31976 all failed because of that test failing.

On my local Windows computer, if I build your commit and run the FormatTest unit test multiple times (the same binary without changing anything or rebuilding), that above test fails roughly 50% of the time.

Can you take a look to figure out why your change might be causing this instability?

Hi,
That is very strange indeed, nothing is supposed to be random here...
I will try to have a look, but I don't have a windows computer to test, and never seen this issue on linux/macos :-/

The linux (s390x arch) buildbot is also failing, http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/31811/steps/ninja%20check%201/logs/stdio.
I reverted it in 47ef09e4848a970c530928496b54085cfdba5a76 to make buildbot happy.

Unfortunately I don't have access to either an s390x or a windows machine; is there a way to trigger a "private" build on buildbot, to use the same setup/build env ?
Or some other way to quickly get the required build env?

(Otherwise, could you please check if adding initialization of ConditionalsLevel to 0 in constructor of WhitespaceManager::Change class (WhitespaceManager.cpp:43).cpp fixes the issue ?)

[dyung]

In D50078#1999183, @Typz wrote:

In D50078#1998578, @hokein wrote:

In D50078#1998520, @Typz wrote:

In D50078#1998398, @dyung wrote:

Hi, this change that you submitted in commit 5daa25fd7a184524759b6ad065a8bd7e95aa149a seems to be causing the test "Clang-Unit :: Format/./FormatTests.exe/FormatTest.ConfigurableUseOfTab" to randomly fail about 50% of the time on Windows.

Take the most recent 5 runs of the llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast buildbot. Builds #31973, 31975 and 31976 all failed because of that test failing.

On my local Windows computer, if I build your commit and run the FormatTest unit test multiple times (the same binary without changing anything or rebuilding), that above test fails roughly 50% of the time.

Can you take a look to figure out why your change might be causing this instability?

Hi,
That is very strange indeed, nothing is supposed to be random here...
I will try to have a look, but I don't have a windows computer to test, and never seen this issue on linux/macos :-/

The linux (s390x arch) buildbot is also failing, http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/31811/steps/ninja%20check%201/logs/stdio.
I reverted it in 47ef09e4848a970c530928496b54085cfdba5a76 to make buildbot happy.

Unfortunately I don't have access to either an s390x or a windows machine; is there a way to trigger a "private" build on buildbot, to use the same setup/build env ?
Or some other way to quickly get the required build env?

(Otherwise, could you please check if adding initialization of ConditionalsLevel to 0 in constructor of WhitespaceManager::Change class (WhitespaceManager.cpp:43).cpp fixes the issue ?)

I tried adding the fix you suggested to WhitespaceManager.cpp, and unfortunately it does not seem to fix the problem. The test still failed 5/10 times on my machine.

[Typz]

Thanks for trying the patch, too bad it does not work...
I will see how to get the required setup/env, but it will probably take some time (esp. in the current situation).
Thanks again

[Typz]

Still no chance getting a windows or s390x machine, but i could reproduce the/a problem systematically on linux by enabling sanitizer.
In this case, initializing ConditionalsLevel to 0 in constructor does index fix this issue (and initializing to 1 ensures the issue happens, though not in "regular" linux build).

[Typz]

Fix random crash on windows

[MyDeveloperDay]

https://bugs.llvm.org/show_bug.cgi?id=33896 shows an interesting use case (when using UseTab: Always)

std::string CLogView::GetItemText(int item) const {
  return item == 1 ? "one" :
         item == 2 ? "two" :
         item == 3 ? "three" :
         item == 4 ? "four" :
                           "high";
}

[srj]

I think the new formatting is unquestionably better than the old formatting.

But...

This change means that the output from clang-format-11 can't be made compatible with older versions of clang-format, which is also unfortunate.

IMHO it should be a goal to have "breaking" format changes like this always be opt-in via a new setting (or a new option for an existing setting).

[MyDeveloperDay]

I get the point about Opt In, and if we are going to add an option it needs to go in ASAP otherwise too many people will then start complaining it was on by default before.

To be honest I think the question is do we consider this a bug or a feature, because we DO NOT provide Opt In on bug fixes (and there is no Opt Out).

If its a bug, there is no need for us to not have this on by default, its better to drive the improvement than 10,000's of .clang-format files needing to be updated just to get reasonable behavior.

[Typz]

In D50078#2050623, @MyDeveloperDay wrote:

I get the point about Opt In, and if we are going to add an option it needs to go in ASAP otherwise too many people will then start complaining it was on by default before.

To be honest I think the question is do we consider this a bug or a feature, because we DO NOT provide Opt In on bug fixes (and there is no Opt Out).

If its a bug, there is no need for us to not have this on by default, its better to drive the improvement than 10,000's of .clang-format files needing to be updated just to get reasonable behavior.

I certainly a bit biased, but I think it is more a bug: it should have been there from the start :-)

And I also think it should be on by default instead of modifying many .clang-format files. So IMHO if we add an option, it should be opt-out.

[krasimir]

I'm happy with this patch!

We found a couple of rough edges:

• alignment after ?: and
• new formatting of _ ? _ ? _ : _ : _ patterns is bad

These are illustrated as examples D and E below (A, B and C look good to me). test.cc is how I'd expect clang-format to behave with this patch with BreakBeforeTernaryOperators = true, like in LLVM style.

• alignment after ?:: this is a GNU extension and we've seen it used in C++ and ObjC: https://stackoverflow.com/questions/24538096/in-objective-c. I think this is special enough for us to consider an occurrence of ?: to break a ternary operator chain for the purposes of this alignment. I'd expect a +4 alignment of the last 2 lines of example D for consistency with A.

• new formatting doesn't work for _ ? _ ? _ : _ : _ patterns; old formatting is better in that case. I think the new chained alignment works very well for _ ? _ : _ ? _ : _ ... cases, but we should attempt it otherwise.

% cat test.cc
//-- column 80 ----------------------------------------------------------------V
// A
int i = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa 
            ? bbbbbbbbbbbbbbbbbbb
            : cccccccccccccccccccc;
 
//-- column 80 ----------------------------------------------------------------V
// B
int i = aaaaaaaaaaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbbb
        : cccccccccccccccccccc     ? dddddddddddddddddddd
                                   : eeeeeeeeeeeeeeeee;
 
//-- column 80 ----------------------------------------------------------------V
// C
int i = aaaaaaaaaaaaaaaaaaaaaaaaaa
            ?: bbbbbbbbbbbbbbbbbbb ? ccccccccccccc
                                   : ddddddddd;
 
//-- column 80 ----------------------------------------------------------------V
// D
int i = aaaaaaaaaaaaaaaaaaaaaaaaaa
            ?: bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
                   ? bbbbbbbbbbbbbbbbbbb
                   : cccccccccccccccccccc;
 
//-- column 80 ----------------------------------------------------------------V
// E 
return temp[0] > temp[1] ? temp[0] > temp[2] ? 0 : 2
                         : temp[1] > temp[2] ? 1 : 2;

% bin/clang-format -style=llvm test.cc
//-- column 80 ----------------------------------------------------------------V
// A
int i = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
            ? bbbbbbbbbbbbbbbbbbb
            : cccccccccccccccccccc;
 
//-- column 80 ----------------------------------------------------------------V
// B
int i = aaaaaaaaaaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbbb
        : cccccccccccccccccccc     ? dddddddddddddddddddd
                                   : eeeeeeeeeeeeeeeee;
 
//-- column 80 ----------------------------------------------------------------V
// C
int i = aaaaaaaaaaaaaaaaaaaaaaaaaa
            ?: bbbbbbbbbbbbbbbbbbb ? ccccccccccccc
                                   : ddddddddd;
 
//-- column 80 ----------------------------------------------------------------V
// D
int i = aaaaaaaaaaaaaaaaaaaaaaaaaa
            ?: bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
               ? bbbbbbbbbbbbbbbbbbb
               : cccccccccccccccccccc;
 
//-- column 80 ----------------------------------------------------------------V
// E
return temp[0] > temp[1]   ? temp[0] > temp[2] ? 0 : 2
       : temp[1] > temp[2] ? 1
                           : 2;

[srj]

And I also think it should be on by default instead of modifying many .clang-format files. So IMHO if we add an option, it should be opt-out.

I can live with it being opt-out. My big concern is that (as it currently stands) our project gets different 'canonical' clang-format results depending on whether people are using LLVM10 vs LLVM11 (and both are common and supported by our project), which is a headache. I just want a way to get consistent, predictable formatting regardless of the clang-format version.

[Typz]

In D50078#2055227, @srj wrote:

And I also think it should be on by default instead of modifying many .clang-format files. So IMHO if we add an option, it should be opt-out.

I can live with it being opt-out. My big concern is that (as it currently stands) our project gets different 'canonical' clang-format results depending on whether people are using LLVM10 vs LLVM11 (and both are common and supported by our project), which is a headache. I just want a way to get consistent, predictable formatting regardless of the clang-format version.

Adding an option would be an issue there, I think: since clang-format is quite strict regarding options, the option would need to exist in both LLVM10 and LLVM11 if you need to support such use-case...

[klimek]

For the policy question: clang-format does intentionally not try to be stable - the "how to" suggestion for clang-format has always been to format changes lines and live with divergence (divergence from people manually formatting things is larger).

[MyDeveloperDay]

In D50078#2055609, @Typz wrote:

In D50078#2055227, @srj wrote:

And I also think it should be on by default instead of modifying many .clang-format files. So IMHO if we add an option, it should be opt-out.

I can live with it being opt-out. My big concern is that (as it currently stands) our project gets different 'canonical' clang-format results depending on whether people are using LLVM10 vs LLVM11 (and both are common and supported by our project), which is a headache. I just want a way to get consistent, predictable formatting regardless of the clang-format version.

Adding an option would be an issue there, I think: since clang-format is quite strict regarding options, the option would need to exist in both LLVM10 and LLVM11 if you need to support such use-case...

The problem here is not that clang-format 10 and 11 are different because they are already different for other bug fixes and we are not providing version to version emulation of bugs just because someone wants to use clang-format-10 and clang-format-11 in the same environment.

The problem is that this team is allowing their developers to mix and match versions, its not for clang-format to solve that problem, that is one of process, a process that could easily be different for other teams (they could be using 7,8,9,10) in such a circumstance we'd say "pick one" the same needs to be true here.

[krasimir]

In D50078#2053324, @krasimir wrote:

I'm happy with this patch!

We found a couple of rough edges:

• alignment after ?: and
• new formatting of _ ? _ ? _ : _ : _ patterns is bad

These are illustrated as examples D and E below (A, B and C look good to me). test.cc is how I'd expect clang-format to behave with this patch with BreakBeforeTernaryOperators = true, like in LLVM style.

• alignment after ?:: this is a GNU extension and we've seen it used in C++ and ObjC: https://stackoverflow.com/questions/24538096/in-objective-c. I think this is special enough for us to consider an occurrence of ?: to break a ternary operator chain for the purposes of this alignment. I'd expect a +4 alignment of the last 2 lines of example D for consistency with A.

• new formatting doesn't work for _ ? _ ? _ : _ : _ patterns; old formatting is better in that case. I think the new chained alignment works very well for _ ? _ : _ ? _ : _ ... cases, but we should attempt it otherwise.

% cat test.cc
//-- column 80 ----------------------------------------------------------------V
// A
int i = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa 
            ? bbbbbbbbbbbbbbbbbbb
            : cccccccccccccccccccc;
 
//-- column 80 ----------------------------------------------------------------V
// B
int i = aaaaaaaaaaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbbb
        : cccccccccccccccccccc     ? dddddddddddddddddddd
                                   : eeeeeeeeeeeeeeeee;
 
//-- column 80 ----------------------------------------------------------------V
// C
int i = aaaaaaaaaaaaaaaaaaaaaaaaaa
            ?: bbbbbbbbbbbbbbbbbbb ? ccccccccccccc
                                   : ddddddddd;
 
//-- column 80 ----------------------------------------------------------------V
// D
int i = aaaaaaaaaaaaaaaaaaaaaaaaaa
            ?: bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
                   ? bbbbbbbbbbbbbbbbbbb
                   : cccccccccccccccccccc;
 
//-- column 80 ----------------------------------------------------------------V
// E 
return temp[0] > temp[1] ? temp[0] > temp[2] ? 0 : 2
                         : temp[1] > temp[2] ? 1 : 2;

% bin/clang-format -style=llvm test.cc
//-- column 80 ----------------------------------------------------------------V
// A
int i = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
            ? bbbbbbbbbbbbbbbbbbb
            : cccccccccccccccccccc;
 
//-- column 80 ----------------------------------------------------------------V
// B
int i = aaaaaaaaaaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbbb
        : cccccccccccccccccccc     ? dddddddddddddddddddd
                                   : eeeeeeeeeeeeeeeee;
 
//-- column 80 ----------------------------------------------------------------V
// C
int i = aaaaaaaaaaaaaaaaaaaaaaaaaa
            ?: bbbbbbbbbbbbbbbbbbb ? ccccccccccccc
                                   : ddddddddd;
 
//-- column 80 ----------------------------------------------------------------V
// D
int i = aaaaaaaaaaaaaaaaaaaaaaaaaa
            ?: bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
               ? bbbbbbbbbbbbbbbbbbb
               : cccccccccccccccccccc;
 
//-- column 80 ----------------------------------------------------------------V
// E
return temp[0] > temp[1]   ? temp[0] > temp[2] ? 0 : 2
       : temp[1] > temp[2] ? 1
                           : 2;

@MyDeveloperDay , @Typz -- could you please take a look at these two issues and give your feedback? (E) looks pretty bad. I'm not familiar enough with this patch to judge how easy / hard would it be to mitigate these.

[MyDeveloperDay]

@MyDeveloperDay , @Typz -- could you please take a look at these two issues and give your feedback? (E) looks pretty bad. I'm not familiar enough with this patch to judge how easy / hard would it be to mitigate these.

I agree 'E' is a regression from

return temp[0] > temp[1] ? temp[0] > temp[2] ? 0 : 2
                         : temp[1] > temp[2] ? 1 : 2;

to

return temp[0] > temp[1]   ? temp[0] > temp[2] ? 0 : 2
       : temp[1] > temp[2] ? 1
                           : 2;

[MyDeveloperDay]

I wondered if it was the < and [] causing issues

return aaaaaaagggbbbbbbb ? ccccccceeeddddddd ? 0 : 2
                         : fffffffffffffffffff ? 1 : 2;

but it seem its not

return aaaaaaagggbbbbbbb     ? ccccccceeeddddddd ? 0 : 2
       : fffffffffffffffffff ? 1
                             : 2;

[Typz]

Indeed, I saw the emails, but I didn't have time to check or investigate the issues.

As for E, this is more tricky than this. At the moment, the code does not look at what is in the first "branch" of the ternary operator : it does not care if this is a nested ternary operator as well... (which would probably add *many* more cases to handle). So, at the moment,

return temp[0] > temp[1]   ? temp[0] > temp[2] ? 0 : 2
       : temp[1] > temp[2] ? 1
                           : 2;

is equivalent to:

return temp[0] > temp[1]   ? myFieldWithVeryLongName
       : temp[1] > temp[2] ? 1
                           : 2;

which looks fine....

At some point, once I had a working change I tried to improve it so that the decision would not be "hardcoded" but penalty-based, but I did not succeed in this.

In this specific case, once option may be to introduce a special case for "balanced ternary operator", e.g. at least in the most simple case (e.g. a ? b ? c : d : e ? f : g), though there may be a more generalized form... Not sure exactly how easy/complicated it would be, though...

[Typz]

Regarding ?:, this is definitely not considered, and should be relatively easy to handle.

Regarding case D, this looks like a bug... The break in the first operand is kind of unexpected (as in "not the usual case"), and I had many such corner cases while making and testing the change. I thought I finally had a proper solution, seems there was still something, though I am not sure it is related to ?: or could happen in other cases (on second thought, it may indeed be related, as there is no operand to align with...)

clang/lib/Format/ContinuationIndenter.h

ParenState(const FormatToken *Tok, unsigned Indent, unsigned LastSpace,
       : Tok(Tok), Indent(Indent), LastSpace(LastSpace),
         NestedBlockIndent(Indent), IsAligned(false),
         BreakBeforeClosingBrace(false), AvoidBinPacking(AvoidBinPacking),
         BreakBeforeParameter(false), NoLineBreak(NoLineBreak),
         NoLineBreakInOperand(false), LastOperatorWrapped(true),
         ContainsLineBreak(false), ContainsUnwrappedBuilder(false),
         AlignColons(true), ObjCSelectorNameFound(false),
         HasMultipleNestedBlocks(false), NestedBlockInlined(false),
-        IsInsideObjCArrayLiteral(false), IsCSharpGenericTypeConstraint(false) {}
+        IsInsideObjCArrayLiteral(false), IsCSharpGenericTypeConstraint(false),
+        IsChainedConditional(false), IsWrappedConditional(false) {}
 
   /// \brief The token opening this parenthesis level, or nullptr if this level
   /// is opened by fake parenthesis.
   ///
   /// Not considered for memoization as it will always have the same value at
   /// the same token.
   const FormatToken *Tok;
 
struct ParenState {
   bool NestedBlockInlined : 1;
 
   /// \c true if the current \c ParenState represents an Objective-C
   /// array literal.
   bool IsInsideObjCArrayLiteral : 1;
 
   bool IsCSharpGenericTypeConstraint : 1;
 
+  /// \brief true if the current \c ParenState represents the false branch of
+  /// a chained conditional expression (e.g. else-if)
+  bool IsChainedConditional : 1;
+
+  /// \brief true if there conditionnal was wrapped on the first operator (the
+  /// question mark)
+  bool IsWrappedConditional : 1;
+
   bool operator<(const ParenState &Other) const {
     if (Indent != Other.Indent)
       return Indent < Other.Indent;
     if (LastSpace != Other.LastSpace)
       return LastSpace < Other.LastSpace;
     if (NestedBlockIndent != Other.NestedBlockIndent)
       return NestedBlockIndent < Other.NestedBlockIndent;
     if (FirstLessLess != Other.FirstLessLess)
bool operator<(const ParenState &Other) const {
     if (ContainsLineBreak != Other.ContainsLineBreak)
       return ContainsLineBreak;
     if (ContainsUnwrappedBuilder != Other.ContainsUnwrappedBuilder)
       return ContainsUnwrappedBuilder;
     if (NestedBlockInlined != Other.NestedBlockInlined)
       return NestedBlockInlined;
     if (IsCSharpGenericTypeConstraint != Other.IsCSharpGenericTypeConstraint)
       return IsCSharpGenericTypeConstraint;
+    if (IsChainedConditional != Other.IsChainedConditional)
+      return IsChainedConditional;
+    if (IsWrappedConditional != Other.IsWrappedConditional)
+      return IsWrappedConditional;
     return false;
   }
 };
 
 /// The current state when indenting a unwrapped line.
 ///
 /// As the indenting tries different combinations this is copied by value.
 struct LineState {

clang/lib/Format/ContinuationIndenter.cpp

if ((startsNextParameter(Current, Style) || Previous.is(tok::semi) ||
         (Previous.NestingLevel == 1 || Style.BinPackParameters)) ||
        (Style.BreakBeforeTernaryOperators && Current.is(TT_ConditionalExpr) &&
         Previous.isNot(tok::question)) ||
        (!Style.BreakBeforeTernaryOperators &&
         Previous.is(TT_ConditionalExpr))) &&
       State.Stack.back().BreakBeforeParameter && !Current.isTrailingComment() &&
       !Current.isOneOf(tok::r_paren, tok::r_brace))
     return true;
+  if (State.Stack.back().IsChainedConditional &&
+      ((Style.BreakBeforeTernaryOperators && Current.is(TT_ConditionalExpr) &&
+        Current.is(tok::colon)) ||
+       (!Style.BreakBeforeTernaryOperators && Previous.is(TT_ConditionalExpr) &&
+        Previous.is(tok::colon))))
+    return true;
   if (((Previous.is(TT_DictLiteral) && Previous.is(tok::l_brace)) ||
        (Previous.is(TT_ArrayInitializerLSquare) &&
         Previous.ParameterCount > 1) ||
        opensProtoMessageField(Previous, Style)) &&
       Style.ColumnLimit > 0 &&
       getLengthToMatchingParen(Previous, State.Stack) + State.Column - 1 >
           getColumnLimit(State))
     return true;
unsigned ContinuationIndenter::getNewLineColumn(const LineState &State) {
   if (NextNonComment->isMemberAccess()) {
     if (State.Stack.back().CallContinuation == 0)
       return ContinuationIndent;
     return State.Stack.back().CallContinuation;
   }
   if (State.Stack.back().QuestionColumn != 0 &&
       ((NextNonComment->is(tok::colon) &&
         NextNonComment->is(TT_ConditionalExpr)) ||
-       Previous.is(TT_ConditionalExpr)))
+       Previous.is(TT_ConditionalExpr))) {
+    if (((NextNonComment->is(tok::colon) && NextNonComment->Next &&
+          !NextNonComment->Next->FakeLParens.empty() &&
+          NextNonComment->Next->FakeLParens.back() == prec::Conditional) ||
+         (Previous.is(tok::colon) && !Current.FakeLParens.empty() &&
+          Current.FakeLParens.back() == prec::Conditional)) &&
+        !State.Stack.back().IsWrappedConditional) {
+      // NOTE: we may tweak this slightly:
+      //    * not remove the 'lead' ContinuationIndentWidth
+      //    * always un-indent by the operator when
+      //    BreakBeforeTernaryOperators=true
+      unsigned Indent =
+          State.Stack.back().Indent - Style.ContinuationIndentWidth;
+      return Indent;
+    }
     return State.Stack.back().QuestionColumn;
+  }
   if (Previous.is(tok::comma) && State.Stack.back().VariablePos != 0)
     return State.Stack.back().VariablePos;
   if ((PreviousNonComment &&
        (PreviousNonComment->ClosesTemplateDeclaration ||
         PreviousNonComment->isOneOf(
             TT_AttributeParen, TT_AttributeSquare, TT_FunctionAnnotationRParen,
             TT_JavaAnnotation, TT_LeadingJavaAnnotation))) ||
       (!Style.IndentWrappedFunctionNames &&
unsigned ContinuationIndenter::moveStateToNextToken(LineState &State,
   if (Current.is(TT_BinaryOperator) && Current.isNot(tok::lessless))
     State.Stack.back().LastOperatorWrapped = Newline;
   if (Current.is(TT_ConditionalExpr) && Current.Previous &&
       !Current.Previous->is(TT_ConditionalExpr))
     State.Stack.back().LastOperatorWrapped = Newline;
   if (Current.is(TT_ArraySubscriptLSquare) &&
       State.Stack.back().StartOfArraySubscripts == 0)
     State.Stack.back().StartOfArraySubscripts = State.Column;
+  if (Current.is(TT_ConditionalExpr) && Current.is(tok::question) &&
+      ((Current.MustBreakBefore) ||
+       (Current.getNextNonComment() &&
+        Current.getNextNonComment()->MustBreakBefore)))
+    State.Stack.back().IsWrappedConditional = true;
   if (Style.BreakBeforeTernaryOperators && Current.is(tok::question))
     State.Stack.back().QuestionColumn = State.Column;
   if (!Style.BreakBeforeTernaryOperators && Current.isNot(tok::colon)) {
     const FormatToken *Previous = Current.Previous;
     while (Previous && Previous->isTrailingComment())
       Previous = Previous->Previous;
     if (Previous && Previous->is(tok::question))
       State.Stack.back().QuestionColumn = State.Column;
void ContinuationIndenter::moveStatePastFakeLParens(LineState &State,
   for (SmallVectorImpl<prec::Level>::const_reverse_iterator
            I = Current.FakeLParens.rbegin(),
            E = Current.FakeLParens.rend();
        I != E; ++I) {
     ParenState NewParenState = State.Stack.back();
     NewParenState.Tok = nullptr;
     NewParenState.ContainsLineBreak = false;
     NewParenState.LastOperatorWrapped = true;
+    NewParenState.IsChainedConditional = false;
+    NewParenState.IsWrappedConditional = false;
     NewParenState.NoLineBreak =
         NewParenState.NoLineBreak || State.Stack.back().NoLineBreakInOperand;
 
     // Don't propagate AvoidBinPacking into subexpressions of arg/param lists.
     if (*I > prec::Comma)
       NewParenState.AvoidBinPacking = false;
 
     // Indent from 'LastSpace' unless these are fake parentheses encapsulating
for (SmallVectorImpl<prec::Level>::const_reverse_iterator
     //   OuterFunction(SomeObject.InnerFunctionCall( // break
     //       ParameterToInnerFunction));
     if (*I > prec::Unknown)
       NewParenState.LastSpace = std::max(NewParenState.LastSpace, State.Column);
     if (*I != prec::Conditional && !Current.is(TT_UnaryOperator) &&
         Style.AlignAfterOpenBracket != FormatStyle::BAS_DontAlign)
       NewParenState.StartOfFunctionCall = State.Column;
 
-    // Always indent conditional expressions. Never indent expression where
-    // the 'operator' is ',', ';' or an assignment (i.e. *I <=
-    // prec::Assignment) as those have different indentation rules. Indent
-    // other expression, unless the indentation needs to be skipped.
-    if (*I == prec::Conditional ||
+    // Indent conditional expressions, unless they are chained "else-if"
+    // conditionals. Never indent expression where the 'operator' is ',', ';' or
+    // an assignment (i.e. *I <= prec::Assignment) as those have different
+    // indentation rules. Indent other expression, unless the indentation needs
+    // to be skipped.
+    if (*I == prec::Conditional && Previous && Previous->is(tok::colon) &&
+        Previous->is(TT_ConditionalExpr) && I == Current.FakeLParens.rbegin() &&
+        !State.Stack.back().IsWrappedConditional) {
+      NewParenState.IsChainedConditional = true;
+    } else if (*I == prec::Conditional ||
                (!SkipFirstExtraIndent && *I > prec::Assignment &&
-         !Current.isTrailingComment()))
+                !Current.isTrailingComment())) {
       NewParenState.Indent += Style.ContinuationIndentWidth;
+    }
     if ((Previous && !Previous->opensScope()) || *I != prec::Comma)
       NewParenState.BreakBeforeParameter = false;
     State.Stack.push_back(NewParenState);
     SkipFirstExtraIndent = false;
   }
 }
 
 void ContinuationIndenter::moveStatePastFakeRParens(LineState &State) {

clang/lib/Format/WhitespaceManager.h

 
 #ifndef LLVM_CLANG_LIB_FORMAT_WHITESPACEMANAGER_H
 #define LLVM_CLANG_LIB_FORMAT_WHITESPACEMANAGER_H
 
 #include "TokenAnnotator.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Format/Format.h"
 #include <string>
+#include <tuple>
 
 namespace clang {
 namespace format {
 
 /// Manages the whitespaces around tokens and their replacements.
 ///
 /// This includes special handling for certain constructs, e.g. the alignment of
 /// trailing line comments.
struct Change {
     // If this Change represents a continuation of a block comment,
     // \c StartOfBlockComment is pointer to the first Change in the block
     // comment. \c IndentationOffset is a relative column offset to this
     // change, so that the correct column can be reconstructed at the end of
     // the alignment process.
     const Change *StartOfBlockComment;
     int IndentationOffset;
 
-    // A combination of indent level and nesting level, which are used in
-    // tandem to compute lexical scope, for the purposes of deciding
+    // Depth of conditionals. Computed from tracking fake parenthesis, except
+    // it does not increase the indent for "chained" conditionals.
+    int ConditionalsLevel;
+
+    // A combination of indent, nesting and conditionals levels, which are used
+    // in tandem to compute lexical scope, for the purposes of deciding
     // when to stop consecutive alignment runs.
-    std::pair<unsigned, unsigned> indentAndNestingLevel() const {
-      return std::make_pair(Tok->IndentLevel, Tok->NestingLevel);
+    std::tuple<unsigned, unsigned, unsigned> indentAndNestingLevel() const {
+      return std::make_tuple(Tok->IndentLevel, Tok->NestingLevel,
+                             ConditionalsLevel);
     }
   };
 
 private:
   /// Calculate \c IsTrailingComment, \c TokenLength for the last tokens
   /// or token parts in a line and \c PreviousEndOfTokenColumn and
   /// \c EscapedNewlineColumn for the first tokens or token parts in a line.
   void calculateLineBreakInformation();
 
   /// \brief Align consecutive C/C++ preprocessor macros over all \c Changes.
   void alignConsecutiveMacros();
 
   /// Align consecutive assignments over all \c Changes.
   void alignConsecutiveAssignments();
 
   /// Align consecutive declarations over all \c Changes.
   void alignConsecutiveDeclarations();
 
+  /// Align consecutive declarations over all \c Changes.
+  void alignChainedConditionals();
+
   /// Align trailing comments over all \c Changes.
   void alignTrailingComments();
 
   /// Align trailing comments from change \p Start to change \p End at
   /// the specified \p Column.
   void alignTrailingComments(unsigned Start, unsigned End, unsigned Column);
 
   /// Align escaped newlines over all \c Changes.

clang/lib/Format/WhitespaceManager.cpp

WhitespaceManager::Change::Change(const FormatToken &Tok,
     : Tok(&Tok), CreateReplacement(CreateReplacement),
       OriginalWhitespaceRange(OriginalWhitespaceRange),
       StartOfTokenColumn(StartOfTokenColumn), NewlinesBefore(NewlinesBefore),
       PreviousLinePostfix(PreviousLinePostfix),
       CurrentLinePrefix(CurrentLinePrefix), IsAligned(IsAligned),
       ContinuesPPDirective(ContinuesPPDirective), Spaces(Spaces),
       IsInsideToken(IsInsideToken), IsTrailingComment(false), TokenLength(0),
       PreviousEndOfTokenColumn(0), EscapedNewlineColumn(0),
-      StartOfBlockComment(nullptr), IndentationOffset(0) {}
+      StartOfBlockComment(nullptr), IndentationOffset(0), ConditionalsLevel(0) {
+}
 
 void WhitespaceManager::replaceWhitespace(FormatToken &Tok, unsigned Newlines,
                                           unsigned Spaces,
                                           unsigned StartOfTokenColumn,
                                           bool IsAligned, bool InPPDirective) {
   if (Tok.Finalized)
     return;
   Tok.Decision = (Newlines > 0) ? FD_Break : FD_Continue;
const tooling::Replacements &WhitespaceManager::generateReplacements() {
   if (Changes.empty())
     return Replaces;
 
   llvm::sort(Changes, Change::IsBeforeInFile(SourceMgr));
   calculateLineBreakInformation();
   alignConsecutiveMacros();
   alignConsecutiveDeclarations();
   alignConsecutiveAssignments();
+  alignChainedConditionals();
   alignTrailingComments();
   alignEscapedNewlines();
   generateChanges();
 
   return Replaces;
 }
 
 void WhitespaceManager::calculateLineBreakInformation() {
if (Change.Tok->is(tok::comment)) {
           Change.IndentationOffset =
               Change.StartOfTokenColumn -
               Change.StartOfBlockComment->StartOfTokenColumn;
       }
     } else {
       LastBlockComment = nullptr;
     }
   }
+
+  // Compute conditional nesting level
+  // Level is increased for each conditional, unless this conditional continues
+  // a chain of conditional, i.e. starts immediately after the colon of another
+  // conditional.
+  SmallVector<bool, 16> ScopeStack;
+  int ConditionalsLevel = 0;
+  for (auto &Change : Changes) {
+    for (unsigned i = 0, e = Change.Tok->FakeLParens.size(); i != e; ++i) {
+      bool isNestedConditional =
+          Change.Tok->FakeLParens[e - 1 - i] == prec::Conditional &&
+          !(i == 0 && Change.Tok->Previous &&
+            Change.Tok->Previous->is(TT_ConditionalExpr) &&
+            Change.Tok->Previous->is(tok::colon));
+      if (isNestedConditional)
+        ++ConditionalsLevel;
+      ScopeStack.push_back(isNestedConditional);
+    }
+
+    Change.ConditionalsLevel = ConditionalsLevel;
+
+    for (unsigned i = Change.Tok->FakeRParens; i > 0 && ScopeStack.size();
+         --i) {
+      if (ScopeStack.pop_back_val())
+        --ConditionalsLevel;
+    }
+  }
 }
 
 // Align a single sequence of tokens, see AlignTokens below.
 template <typename F>
 static void
 AlignTokenSequence(unsigned Start, unsigned End, unsigned Column, F &&Matches,
                    SmallVector<WhitespaceManager::Change, 16> &Changes) {
   bool FoundMatchOnLine = false;
   int Shift = 0;
 
   // ScopeStack keeps track of the current scope depth. It contains indices of
   // the first token on each scope.
   // We only run the "Matches" function on tokens from the outer-most scope.
   // However, we do need to pay special attention to one class of tokens
   // that are not in the outer-most scope, and that is function parameters
   // which are split across multiple lines, as illustrated by this example:
   //   double a(int x);
   //   int    b(int  y,
   //          double z);
   // In the above example, we need to take special care to ensure that
   // 'double z' is indented along with it's owning function 'b'.
+  // Special handling is required for 'nested' ternary operators.
   SmallVector<unsigned, 16> ScopeStack;
 
   for (unsigned i = Start; i != End; ++i) {
     if (ScopeStack.size() != 0 &&
         Changes[i].indentAndNestingLevel() <
             Changes[ScopeStack.back()].indentAndNestingLevel())
       ScopeStack.pop_back();
 
for (unsigned i = Start; i != End; ++i) {
     }
 
     // This is for function parameters that are split across multiple lines,
     // as mentioned in the ScopeStack comment.
     if (InsideNestedScope && Changes[i].NewlinesBefore > 0) {
       unsigned ScopeStart = ScopeStack.back();
       if (Changes[ScopeStart - 1].Tok->is(TT_FunctionDeclarationName) ||
           (ScopeStart > Start + 1 &&
-           Changes[ScopeStart - 2].Tok->is(TT_FunctionDeclarationName)))
+           Changes[ScopeStart - 2].Tok->is(TT_FunctionDeclarationName)) ||
+          Changes[i].Tok->is(TT_ConditionalExpr) ||
+          (Changes[i].Tok->Previous &&
+           Changes[i].Tok->Previous->is(TT_ConditionalExpr)))
         Changes[i].Spaces += Shift;
     }
 
     assert(Shift >= 0);
     Changes[i].StartOfTokenColumn += Shift;
     if (i + 1 != Changes.size())
       Changes[i + 1].PreviousEndOfTokenColumn += Shift;
   }
static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
   // Line number of the start and the end of the current token sequence.
   unsigned StartOfSequence = 0;
   unsigned EndOfSequence = 0;
 
   // Measure the scope level (i.e. depth of (), [], {}) of the first token, and
   // abort when we hit any token in a higher scope than the starting one.
   auto IndentAndNestingLevel = StartAt < Changes.size()
                                    ? Changes[StartAt].indentAndNestingLevel()
-                                   : std::pair<unsigned, unsigned>(0, 0);
+                                   : std::tuple<unsigned, unsigned, unsigned>();
 
   // Keep track of the number of commas before the matching tokens, we will only
   // align a sequence of matching tokens if they are preceded by the same number
   // of commas.
   unsigned CommasBeforeLastMatch = 0;
   unsigned CommasBeforeMatch = 0;
 
   // Whether a matching token has been found on the current line.
for (unsigned e = Changes.size(); i != e; ++i) {
 
     CommasBeforeLastMatch = CommasBeforeMatch;
     FoundMatchOnLine = true;
 
     if (StartOfSequence == 0)
       StartOfSequence = i;
 
     unsigned ChangeMinColumn = Changes[i].StartOfTokenColumn;
-    int LineLengthAfter = -Changes[i].Spaces;
-    for (unsigned j = i; j != e && Changes[j].NewlinesBefore == 0; ++j)
+    int LineLengthAfter = Changes[i].TokenLength;
+    for (unsigned j = i + 1; j != e && Changes[j].NewlinesBefore == 0; ++j)
       LineLengthAfter += Changes[j].Spaces + Changes[j].TokenLength;
     unsigned ChangeMaxColumn = Style.ColumnLimit - LineLengthAfter;
 
     // If we are restricted by the maximum column width, end the sequence.
     if (ChangeMinColumn > MaxColumn || ChangeMaxColumn < MinColumn ||
         CommasBeforeLastMatch != CommasBeforeMatch) {
       AlignCurrentSequence();
       StartOfSequence = i;
AlignTokens(
                             tok::kw_operator))
             return false;
         }
         return true;
       },
       Changes, /*StartAt=*/0);
 }
 
+void WhitespaceManager::alignChainedConditionals() {
+  if (Style.BreakBeforeTernaryOperators) {
+    AlignTokens(
+        Style,
+        [](Change const &C) {
+          // Align question operators and last colon
+          return C.Tok->is(TT_ConditionalExpr) &&
+                 ((C.Tok->is(tok::question) && !C.NewlinesBefore) ||
+                  (C.Tok->is(tok::colon) && C.Tok->Next &&
+                   (C.Tok->Next->FakeLParens.size() == 0 ||
+                    C.Tok->Next->FakeLParens.back() != prec::Conditional)));
+        },
+        Changes, /*StartAt=*/0);
+  } else {
+    static auto AlignWrappedOperand = [](Change const &C) {
+      auto Previous = C.Tok->getPreviousNonComment(); // Previous;
+      return C.NewlinesBefore && Previous && Previous->is(TT_ConditionalExpr) &&
+             (Previous->is(tok::question) ||
+              (Previous->is(tok::colon) &&
+               (C.Tok->FakeLParens.size() == 0 ||
+                C.Tok->FakeLParens.back() != prec::Conditional)));
+    };
+    // Ensure we keep alignment of wrapped operands with non-wrapped operands
+    // Since we actually align the operators, the wrapped operands need the
+    // extra offset to be properly aligned.
+    for (Change &C : Changes) {
+      if (AlignWrappedOperand(C))
+        C.StartOfTokenColumn -= 2;
+    }
+    AlignTokens(
+        Style,
+        [this](Change const &C) {
+          // Align question operators if next operand is not wrapped, as
+          // well as wrapped operands after question operator or last
+          // colon in conditional sequence
+          return (C.Tok->is(TT_ConditionalExpr) && C.Tok->is(tok::question) &&
+                  &C != &Changes.back() && (&C + 1)->NewlinesBefore == 0 &&
+                  !(&C + 1)->IsTrailingComment) ||
+                 AlignWrappedOperand(C);
+        },
+        Changes, /*StartAt=*/0);
+  }
+}
+
 void WhitespaceManager::alignTrailingComments() {
   unsigned MinColumn = 0;
   unsigned MaxColumn = UINT_MAX;
   unsigned StartOfSequence = 0;
   bool BreakBeforeNext = false;
   unsigned Newlines = 0;
   for (unsigned i = 0, e = Changes.size(); i != e; ++i) {
     if (Changes[i].StartOfBlockComment)

clang/unittests/Format/FormatTest.cpp

verifyFormat("unsigned Indent =\n"
                "           TheLine.InPPDirective, PreviousEndOfLineColumn);",
                getLLVMStyleWithColumns(60));
   verifyFormat("bool aaaaaa = aaaaaaaaaaaaa //\n"
                "                  ? aaaaaaaaaaaaaaa\n"
                "                  : bbbbbbbbbbbbbbb //\n"
                "                        ? ccccccccccccccc\n"
                "                        : ddddddddddddddd;");
   verifyFormat("bool aaaaaa = aaaaaaaaaaaaa //\n"
                "                  ? aaaaaaaaaaaaaaa\n"
                "                  : (bbbbbbbbbbbbbbb //\n"
                "                         ? ccccccccccccccc\n"
                "                         : ddddddddddddddd);");
   verifyFormat(
       "int aaaaaaaaaaaaaaaaaaaaaaaaaaa = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
       "                                      ? aaaaaaaaaaaaaaaaaaaaaaaaa +\n"
       "                                            aaaaaaaaaaaaaaaaaaaaa +\n"
       "                                            aaaaaaaaaaaaaaaaaaaaa\n"
verifyFormat("return a != b\n"
                "           : a;\n");
   verifyFormat("return a != b\n"
                "           // comment\n"
                "           ? a\n"
                "           : a = a != b\n"
                "                     // comment\n"
                "                     ? a = b\n"
                "                     : a;");
+
+  // Chained conditionals
+  FormatStyle Style = getLLVMStyle();
+  Style.ColumnLimit = 70;
+  Style.AlignOperands = true;
+  verifyFormat("return aaaaaaaaaaaaaaaa ? 1111111111111111\n"
+               "       : bbbbbbbbbbbbbb ? 2222222222222222\n"
+               "                        : 3333333333333333;",
+               Style);
+  verifyFormat("return aaaaaaaaaaaaaaaa ? 1111111111111111\n"
+               "       : bbbbbbbbbb     ? 2222222222222222\n"
+               "                        : 3333333333333333;",
+               Style);
+  verifyFormat("return aaaaaaaaaa         ? 1111111111111111\n"
+               "       : bbbbbbbbbbbbbbbb ? 2222222222222222\n"
+               "                          : 3333333333333333;",
+               Style);
+  verifyFormat("return aaaaaaaaaaaaaaaa ? 1111111111111111\n"
+               "       : bbbbbbbbbbbbbb ? 222222\n"
+               "                        : 333333;",
+               Style);
+  verifyFormat("return aaaaaaaaaaaaaaaa ? 1111111111111111\n"
+               "       : bbbbbbbbbbbbbb ? 2222222222222222\n"
+               "       : cccccccccccccc ? 3333333333333333\n"
+               "                        : 4444444444444444;",
+               Style);
+  verifyFormat("return aaaaaaaaaaaaaaaa ? (aaa ? bbb : ccc)\n"
+               "       : bbbbbbbbbbbbbb ? 2222222222222222\n"
+               "                        : 3333333333333333;",
+               Style);
+  verifyFormat("return aaaaaaaaaaaaaaaa ? 1111111111111111\n"
+               "       : bbbbbbbbbbbbbb ? 2222222222222222\n"
+               "                        : (aaa ? bbb : ccc);",
+               Style);
+  verifyFormat(
+      "return aaaaaaaaaaaaaaaa ? (aaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
+      "                                             : cccccccccccccccccc)\n"
+      "       : bbbbbbbbbbbbbb ? 2222222222222222\n"
+      "                        : 3333333333333333;",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaa        ? (aaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
+      "                                             : cccccccccccccccccc)\n"
+      "       : bbbbbbbbbbbbbb ? 2222222222222222\n"
+      "                        : 3333333333333333;",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaa        ? a = (aaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
+      "                                             : dddddddddddddddddd)\n"
+      "       : bbbbbbbbbbbbbb ? 2222222222222222\n"
+      "                        : 3333333333333333;",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaa        ? a + (aaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
+      "                                             : dddddddddddddddddd)\n"
+      "       : bbbbbbbbbbbbbb ? 2222222222222222\n"
+      "                        : 3333333333333333;",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaa        ? 1111111111111111\n"
+      "       : bbbbbbbbbbbbbb ? 2222222222222222\n"
+      "                        : a + (aaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
+      "                                             : dddddddddddddddddd)\n",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaaaaaaaaa ? 1111111111111111\n"
+      "       : bbbbbbbbbbbbbb ? 2222222222222222\n"
+      "                        : (aaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
+      "                                             : cccccccccccccccccc);",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaaaaaaaaa ? (aaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
+      "                           : ccccccccccccccc ? dddddddddddddddddd\n"
+      "                                             : eeeeeeeeeeeeeeeeee)\n"
+      "       : bbbbbbbbbbbbbb ? 2222222222222222\n"
+      "                        : 3333333333333333;",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaaaaaaaaa ? (aaaaaaaaaaaaaa    ? bbbbbbbbbbbbbbbbbb\n"
+      "                           : ccccccccccccccc ? dddddddddddddddddd\n"
+      "                                             : eeeeeeeeeeeeeeeeee)\n"
+      "       : bbbbbbbbbbbbbb ? 2222222222222222\n"
+      "                        : 3333333333333333;",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaaaaaaaaa ? (aaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
+      "                           : cccccccccccc    ? dddddddddddddddddd\n"
+      "                                             : eeeeeeeeeeeeeeeeee)\n"
+      "       : bbbbbbbbbbbbbb ? 2222222222222222\n"
+      "                        : 3333333333333333;",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaaaaaaaaa ? aaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
+      "                                             : cccccccccccccccccc\n"
+      "       : bbbbbbbbbbbbbb ? 2222222222222222\n"
+      "                        : 3333333333333333;",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaaaaaaaaa ? aaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
+      "                          : cccccccccccccccc ? dddddddddddddddddd\n"
+      "                                             : eeeeeeeeeeeeeeeeee\n"
+      "       : bbbbbbbbbbbbbb ? 2222222222222222\n"
+      "                        : 3333333333333333;",
+      Style);
+  verifyFormat("return aaaaaaaaaaaaaaaaaaaaa\n"
+               "           ? (aaaaaaaaaaaaaaaaaa   ? bbbbbbbbbbbbbbbbbb\n"
+               "              : cccccccccccccccccc ? dddddddddddddddddd\n"
+               "                                   : eeeeeeeeeeeeeeeeee)\n"
+               "       : bbbbbbbbbbbbbbbbbbb ? 2222222222222222\n"
+               "                             : 3333333333333333;",
+               Style);
+  verifyFormat("return aaaaaaaaaaaaaaaaaaaaaaaaa\n"
+               "           ? aaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb\n"
+               "             : cccccccccccccccc ? dddddddddddddddddd\n"
+               "                                : eeeeeeeeeeeeeeeeee\n"
+               "       : bbbbbbbbbbbbbbbbbbbbbbb ? 2222222222222222\n"
+               "                                 : 3333333333333333;",
+               Style);
 }
 
 TEST_F(FormatTest, BreaksConditionalExpressionsAfterOperator) {
   FormatStyle Style = getLLVMStyle();
   Style.BreakBeforeTernaryOperators = false;
   Style.ColumnLimit = 70;
   verifyFormat(
       "aaaa(aaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaaaaaa ?\n"
verifyFormat("unsigned Indent =\n"
                "               TheLine * 2,\n"
                "           TheLine.InPPDirective, PreviousEndOfLineColumn);",
                Style);
   verifyFormat("bool aaaaaa = aaaaaaaaaaaaa ? //\n"
                "                  aaaaaaaaaaaaaaa :\n"
                "                  bbbbbbbbbbbbbbb ? //\n"
                "                      ccccccccccccccc :\n"
                "                      ddddddddddddddd;",
                Style);
   verifyFormat("bool aaaaaa = aaaaaaaaaaaaa ? //\n"
                "                  aaaaaaaaaaaaaaa :\n"
                "                  (bbbbbbbbbbbbbbb ? //\n"
                "                       ccccccccccccccc :\n"
                "                       ddddddddddddddd);",
                Style);
   verifyFormat("int i = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ?\n"
                "            /*bbbbbbbbbbbbbbb=*/bbbbbbbbbbbbbbbbbbbbbbbbb :\n"
                "            ccccccccccccccccccccccccccc;",
                Style);
   verifyFormat("return aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ?\n"
                "           aaaaa :\n"
                "           bbbbbbbbbbbbbbb + cccccccccccccccc;",
                Style);
+
+  // Chained conditionals
+  verifyFormat("return aaaaaaaaaaaaaaaa ? 1111111111111111 :\n"
+               "       bbbbbbbbbbbbbbbb ? 2222222222222222 :\n"
+               "                          3333333333333333;",
+               Style);
+  verifyFormat("return aaaaaaaaaaaaaaaa ? 1111111111111111 :\n"
+               "       bbbbbbbbbb       ? 2222222222222222 :\n"
+               "                          3333333333333333;",
+               Style);
+  verifyFormat("return aaaaaaaaaa       ? 1111111111111111 :\n"
+               "       bbbbbbbbbbbbbbbb ? 2222222222222222 :\n"
+               "                          3333333333333333;",
+               Style);
+  verifyFormat("return aaaaaaaaaaaaaaaa ? 1111111111111111 :\n"
+               "       bbbbbbbbbbbbbbbb ? 222222 :\n"
+               "                          333333;",
+               Style);
+  verifyFormat("return aaaaaaaaaaaaaaaa ? 1111111111111111 :\n"
+               "       bbbbbbbbbbbbbbbb ? 2222222222222222 :\n"
+               "       cccccccccccccccc ? 3333333333333333 :\n"
+               "                          4444444444444444;",
+               Style);
+  verifyFormat("return aaaaaaaaaaaaaaaa ? (aaa ? bbb : ccc) :\n"
+               "       bbbbbbbbbbbbbbbb ? 2222222222222222 :\n"
+               "                          3333333333333333;",
+               Style);
+  verifyFormat("return aaaaaaaaaaaaaaaa ? 1111111111111111 :\n"
+               "       bbbbbbbbbbbbbbbb ? 2222222222222222 :\n"
+               "                          (aaa ? bbb : ccc);",
+               Style);
+  verifyFormat(
+      "return aaaaaaaaaaaaaaaa ? (aaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb :\n"
+      "                                               cccccccccccccccccc) :\n"
+      "       bbbbbbbbbbbbbbbb ? 2222222222222222 :\n"
+      "                          3333333333333333;",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaa        ? (aaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb :\n"
+      "                                               cccccccccccccccccc) :\n"
+      "       bbbbbbbbbbbbbbbb ? 2222222222222222 :\n"
+      "                          3333333333333333;",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaa        ? a = (aaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb :\n"
+      "                                               dddddddddddddddddd) :\n"
+      "       bbbbbbbbbbbbbbbb ? 2222222222222222 :\n"
+      "                          3333333333333333;",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaa        ? a + (aaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb :\n"
+      "                                               dddddddddddddddddd) :\n"
+      "       bbbbbbbbbbbbbbbb ? 2222222222222222 :\n"
+      "                          3333333333333333;",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaa        ? 1111111111111111 :\n"
+      "       bbbbbbbbbbbbbbbb ? 2222222222222222 :\n"
+      "                          a + (aaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb :\n"
+      "                                               dddddddddddddddddd)\n",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaaaaaaaaa ? 1111111111111111 :\n"
+      "       bbbbbbbbbbbbbbbb ? 2222222222222222 :\n"
+      "                          (aaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb :\n"
+      "                                               cccccccccccccccccc);",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaaaaaaaaa ? (aaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb :\n"
+      "                           ccccccccccccccccc ? dddddddddddddddddd :\n"
+      "                                               eeeeeeeeeeeeeeeeee) :\n"
+      "       bbbbbbbbbbbbbbbb ? 2222222222222222 :\n"
+      "                          3333333333333333;",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaaaaaaaaa ? (aaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb :\n"
+      "                           ccccccccccccc     ? dddddddddddddddddd :\n"
+      "                                               eeeeeeeeeeeeeeeeee) :\n"
+      "       bbbbbbbbbbbbbbbb ? 2222222222222222 :\n"
+      "                          3333333333333333;",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaaaaaaaaa ? (aaaaaaaaaaaaa     ? bbbbbbbbbbbbbbbbbb :\n"
+      "                           ccccccccccccccccc ? dddddddddddddddddd :\n"
+      "                                               eeeeeeeeeeeeeeeeee) :\n"
+      "       bbbbbbbbbbbbbbbb ? 2222222222222222 :\n"
+      "                          3333333333333333;",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaaaaaaaaa ? aaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb :\n"
+      "                                               cccccccccccccccccc :\n"
+      "       bbbbbbbbbbbbbbbb ? 2222222222222222 :\n"
+      "                          3333333333333333;",
+      Style);
+  verifyFormat(
+      "return aaaaaaaaaaaaaaaa ? aaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb :\n"
+      "                          cccccccccccccccccc ? dddddddddddddddddd :\n"
+      "                                               eeeeeeeeeeeeeeeeee :\n"
+      "       bbbbbbbbbbbbbbbb ? 2222222222222222 :\n"
+      "                          3333333333333333;",
+      Style);
+  verifyFormat("return aaaaaaaaaaaaaaaaaaaaa ?\n"
+               "           (aaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb :\n"
+               "            cccccccccccccccccc ? dddddddddddddddddd :\n"
+               "                                 eeeeeeeeeeeeeeeeee) :\n"
+               "       bbbbbbbbbbbbbbbbbbbbb ? 2222222222222222 :\n"
+               "                               3333333333333333;",
+               Style);
+  verifyFormat("return aaaaaaaaaaaaaaaaaaaaa ?\n"
+               "           aaaaaaaaaaaaaaaaaaaa ? bbbbbbbbbbbbbbbbbb :\n"
+               "           cccccccccccccccccccc ? dddddddddddddddddd :\n"
+               "                                  eeeeeeeeeeeeeeeeee :\n"
+               "       bbbbbbbbbbbbbbbbbbbbb ? 2222222222222222 :\n"
+               "                               3333333333333333;",
+               Style);
 }
 
 TEST_F(FormatTest, DeclarationsOfMultipleVariables) {
   verifyFormat("bool aaaaaaaaaaaaaaaaa = aaaaaa->aaaaaaaaaaaaaaaaa(),\n"
                "     aaaaaaaaaaa = aaaaaa->aaaaaaaaaaa();");
   verifyFormat("bool a = true, b = false;");
 
   verifyFormat("bool aaaaaaaaaaaaaaaaaaaaaaaaa =\n"