Page MenuHomePhabricator

clang-format: support aligned nested conditionals formatting
ClosedPublic

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 TranscriptMay 22 2019, 9:35 AM
MyDeveloperDay added a project: Restricted Project.Oct 1 2019, 1:49 AM
MyDeveloperDay accepted this revision.Oct 1 2019, 2:37 AM
MyDeveloperDay added a subscriber: 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.

This revision is now accepted and ready to land.Oct 1 2019, 2:37 AM
Typz added a comment.Oct 17 2019, 6:34 AM

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 updated this revision to Diff 226163.Oct 23 2019, 10:01 AM

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

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
5893

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.

6161

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

Typz updated this revision to Diff 226218.Oct 24 2019, 3:05 AM
Typz marked 2 inline comments as done.

Fix corner in previous rebase

Typz updated this revision to Diff 226245.Oct 24 2019, 6:23 AM

Fix earlier error in patch upload.

Typz updated this revision to Diff 226246.EditedOct 24 2019, 6:29 AM

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

clang/unittests/Format/FormatTest.cpp
5893

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
6161

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 updated this revision to Diff 226380.Oct 25 2019, 12:26 AM

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

Typz marked 3 inline comments as done.Oct 25 2019, 12:29 AM
Typz added inline comments.
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 added a subscriber: dyung.EditedApr 22 2020, 9:28 PM

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 added a comment.Apr 22 2020, 11:24 PM

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 marked an inline comment as done.EditedApr 22 2020, 11:33 PM
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 added a subscriber: hokein.Apr 23 2020, 12:30 AM

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 added a comment.Apr 23 2020, 7:26 AM

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

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 added a comment.Apr 24 2020, 3:13 AM

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 added a comment.EditedMay 6 2020, 6:11 AM

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 updated this revision to Diff 264234.May 15 2020, 7:03 AM

Fix random crash on windows

This revision was automatically updated to reflect the committed changes.

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 added a subscriber: srj.May 21 2020, 4:36 PM

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

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 added a comment.May 22 2020, 4:30 AM

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.

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 added a comment.May 26 2020, 9:54 AM

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 added a comment.May 26 2020, 12:42 PM
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...

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

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.

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 , @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;

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 added a comment.May 28 2020, 9:39 AM

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 added a comment.May 28 2020, 9:47 AM

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