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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
