[PATCH] D50078: clang-format: support aligned nested conditionals formatting

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added a comment.

In D50078#2053324 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D50078/new/

https://reviews.llvm.org/D50078