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

Typz added a comment. 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 te

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

Typz added a comment. 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 oper

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

MyDeveloperDay added a comment. I wondered if it was the < and [] causing issues return aaagggbbb ? ccceeeddd ? 0 : 2 : fff ? 1 : 2; but it seem its not return aaagggbbb ? ccceeeddd ? 0 : 2 : fff

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

MyDeveloperDay added a comment. > @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 r

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

krasimir added a comment. 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 exampl

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

MyDeveloperDay added a comment. 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

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

klimek added a comment. 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). Repository: rG LLV

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

Typz added a comment. 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 concer

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

srj added a comment. > 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-f

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

krasimir added a comment. 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 be

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

Typz added a comment. 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

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

MyDeveloperDay added a comment. 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 D

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

srj added a comment. 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" f

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

MyDeveloperDay added a comment. 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" : it

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG4db94094b469: clang-format: support aligned nested conditionals formatting (authored by Typz). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D50078/new/ http

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

Typz updated this revision to Diff 264234. Typz added a comment. Fix random crash on windows Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D50078/new/ https://reviews.llvm.org/D50078 Files: clang/lib/Format/ContinuationIndenter.cpp clang/lib/Fo

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

Typz added a comment. Still no change with 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, t

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

Typz added a comment. 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 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.

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

dyung added a comment. In D50078#1999183 , @Typz wrote: > In D50078#1998578 , @hokein wrote: > > > In D50078#1998520 , @Typz wrote: > > > > > In D50078#1998398

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

Typz added a comment. In D50078#1998578 , @hokein wrote: > In D50078#1998520 , @Typz wrote: > > > In D50078#1998398 , @dyung wrote: > > > > > Hi, this change that you submitt

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

hokein added a comment. In D50078#1998520 , @Typz wrote: > In D50078#1998398 , @dyung wrote: > > > Hi, this change that you submitted in commit > > 5daa25fd7a184524759b6ad065a8bd7e95aa149a > >

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

Typz marked an inline comment as done. Typz added a comment. Typz added inline comments. Comment at: clang/lib/Format/WhitespaceManager.h:163 +// it does not increase the indent for "chained" conditionals. +int ConditionalsLevel; + This field is not initi

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

Typz added a comment. In D50078#1998398 , @dyung wrote: > Hi, this change that you submitted in commit > 5daa25fd7a184524759b6ad065a8bd7e95aa149a > seems > to be causing the test "Cla

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

dyung added a comment. Hi, this change that you submitted in commit 5daa25fd7a184524759b6ad065a8bd7e95aa149a seems to be causing the test "Clang-Unit :: Format/./FormatTests.exe/FormatTest.ConfigurableUseOfTab" to randomly f

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

Typz marked 3 inline comments as done. Typz added inline comments. Comment at: clang/lib/Format/ContinuationIndenter.cpp:1125 + if (Current.is(TT_ConditionalExpr) && Current.is(tok::question) && + ((Current.MustBreakBefore) || + (Current.getNextNonComment() && Curren

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

Typz updated this revision to Diff 226380. Typz added a comment. Implement a fallback to regularly indented alignment when the question mark is wrapped. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D50078/new/ https://reviews.llvm.org/D50078 File

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

Typz added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:5739 + " ? ccc\n" + " : ddd;"); verifyFormat("bool aa = a //\n" MyDeveloperDay w

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

Typz updated this revision to Diff 226246. Typz added a comment. . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D50078/new/ https://reviews.llvm.org/D50078 Files: clang/lib/Format/ContinuationIndenter.cpp clang/lib/Format/ContinuationIndenter.

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

Typz updated this revision to Diff 226245. Typz added a comment. Fix earlier error in patch upload. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D50078/new/ https://reviews.llvm.org/D50078 Files: clang/lib/Format/ContinuationIndenter.cpp clang

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

MyDeveloperDay accepted this revision. MyDeveloperDay added a comment. LGTM 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-com

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

Typz updated this revision to Diff 226218. Typz marked 2 inline comments as done. Typz added a comment. Fix corner in previous rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D50078/new/ https://reviews.llvm.org/D50078 Files: clang/lib/Forma

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

MyDeveloperDay added a comment. 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.

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

Typz updated this revision to Diff 226163. Typz added a comment. Rebased on master, integrating required code from https://reviews.llvm.org/D32478 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D50078/new/ https://reviews.llvm.org/D50078 Files: c

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

Typz added a comment. 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 m

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

MyDeveloperDay accepted this revision. MyDeveloperDay added a comment. This revision is now accepted and ready to land. 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. Repository: rC Clang CHANGES S

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

Typz updated this revision to Diff 204015. Typz added a comment. Rebase Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D50078/new/ https://reviews.llvm.org/D50078 Files: lib/Format/ContinuationIndenter.cpp lib/Format/ContinuationIndenter.h lib/Format/Whitesp

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

Typz added a comment. ping ? Repository: rC Clang 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/lis

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

Typz added a comment. Herald added a project: clang. ping ? Repository: rC Clang 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://lis

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

Typz added a comment. ping ? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D50078/new/ https://reviews.llvm.org/D50078 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/list

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

Typz added a comment. In https://reviews.llvm.org/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

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

Typz added a comment. In https://reviews.llvm.org/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 = ? 0 : ? 1 : ; or:

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

krasimir added a comment. 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 : loocondition ? result2 : dition

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

klimek added a comment. 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 ;) Repository: rC Clang https://reviews.llvm.org/D50078

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

djasper added a comment. 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 BreakBefore

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

Typz added a comment. 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 (https://reviews.llvm.org/D32478), which supports aligning the w

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

Typz created this revision. Typz added reviewers: krasimir, djasper, klimek. Herald added a subscriber: acoomans. 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 i