[PATCH] D82199: [clang-format] restore indent in conditionals when AlignOperands is DontAlign
krasimir marked an inline comment as done.
krasimir added inline comments.
Comment at: clang/unittests/Format/FormatTest.cpp:6287
+ Style.BreakBeforeTernaryOperators = false;
+ verifyFormat("int x = aaa ? aa :\n"
+ "? cc :\n"
sammccall wrote:
> aligning the question marks here is a bit weird (given DontAlign) but that's
> another patch.
>
> If we disable the question-column behavior with dontalign, this patch will be
> completely dead, right?
> May want to add a FIXME to remove in that case.
Added a FIXME. I think disabling question-column alignment with dontalign
indeed can make this patch obsolete.
Comment at: clang/unittests/Format/FormatTest.cpp:6287
+ Style.BreakBeforeTernaryOperators = false;
+ verifyFormat("int x = aaa ? aa :\n"
+ "? cc :\n"
Typz wrote:
> krasimir wrote:
> > sammccall wrote:
> > > aligning the question marks here is a bit weird (given DontAlign) but
> > > that's another patch.
> > >
> > > If we disable the question-column behavior with dontalign, this patch
> > > will be completely dead, right?
> > > May want to add a FIXME to remove in that case.
> > Added a FIXME. I think disabling question-column alignment with dontalign
> > indeed can make this patch obsolete.
> I don't think this is so weird: even with DontAlign, there are other cases
> when some alignment is performed: for exemple when formatting tables in
> column.
>
> As I see it, ternary operator formatting is a similar case of 2D formatting,
> and while it needs indeed to respect the "general" line wrapping/indent mode
> (as per AlignOperands), it is OK to keep the alignment of the ternary
> operator themselves : otherwise, the whole "mode" for ternary operators need
> to be disabled and makes no sense.
>
> But anyway this is a different patch as you mentioned, and maybe some user of
> DontAlign can come up with a better approach for formatting ternary ops in
> that case.
Agree, this is more of a policy / preference on how we want these snippets to
look like with DontAlign. We should discuss it in more detail whenever we
attempt to refine this later. In a way, this touches back on djasper's comment
on D50078, which I believe was left unaddressed:
> 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
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82199/new/
https://reviews.llvm.org/D82199
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82199: [clang-format] restore indent in conditionals when AlignOperands is DontAlign
Typz added inline comments.
Comment at: clang/unittests/Format/FormatTest.cpp:6287
+ Style.BreakBeforeTernaryOperators = false;
+ verifyFormat("int x = aaa ? aa :\n"
+ "? cc :\n"
sammccall wrote:
> aligning the question marks here is a bit weird (given DontAlign) but that's
> another patch.
>
> If we disable the question-column behavior with dontalign, this patch will be
> completely dead, right?
> May want to add a FIXME to remove in that case.
I don't think this is so weird: even with DontAlign, there are other cases when
some alignment is performed: for exemple when formatting tables in column.
As I see it, ternary operator formatting is a similar case of 2D formatting,
and while it needs indeed to respect the "general" line wrapping/indent mode
(as per AlignOperands), it is OK to keep the alignment of the ternary operator
themselves : otherwise, the whole "mode" for ternary operators need to be
disabled and makes no sense.
But anyway this is a different patch as you mentioned, and maybe some user of
DontAlign can come up with a better approach for formatting ternary ops in that
case.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82199/new/
https://reviews.llvm.org/D82199
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82199: [clang-format] restore indent in conditionals when AlignOperands is DontAlign
This revision was automatically updated to reflect the committed changes.
Closed by commit rG0fad648b65b9: [clang-format] restore indent in conditionals
when AlignOperands is DontAlign (authored by krasimir).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82199/new/
https://reviews.llvm.org/D82199
Files:
clang/lib/Format/ContinuationIndenter.cpp
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6281,6 +6281,17 @@
" : bbb ? \n"
" : ;",
Style);
+
+ Style.AlignOperands = FormatStyle::OAS_DontAlign;
+ Style.BreakBeforeTernaryOperators = false;
+ // FIXME: Aligning the question marks is weird given DontAlign.
+ // Consider disabling this alignment in this case. Also check whether this
+ // will render the adjustment from https://reviews.llvm.org/D82199
+ // unnecessary.
+ verifyFormat("int x = aaa ? aa :\n"
+ "? cc :\n"
+ " d;\n",
+ Style);
}
TEST_F(FormatTest, BreaksConditionalExpressionsAfterOperator) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1041,8 +1041,10 @@
//* not remove the 'lead' ContinuationIndentWidth
//* always un-indent by the operator when
//BreakBeforeTernaryOperators=true
- unsigned Indent =
- State.Stack.back().Indent - Style.ContinuationIndentWidth;
+ unsigned Indent = State.Stack.back().Indent;
+ if (Style.AlignOperands != FormatStyle::OAS_DontAlign) {
+Indent -= Style.ContinuationIndentWidth;
+ }
if (Style.BreakBeforeTernaryOperators &&
State.Stack.back().UnindentOperator)
Indent -= 2;
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6281,6 +6281,17 @@
" : bbb ? \n"
" : ;",
Style);
+
+ Style.AlignOperands = FormatStyle::OAS_DontAlign;
+ Style.BreakBeforeTernaryOperators = false;
+ // FIXME: Aligning the question marks is weird given DontAlign.
+ // Consider disabling this alignment in this case. Also check whether this
+ // will render the adjustment from https://reviews.llvm.org/D82199
+ // unnecessary.
+ verifyFormat("int x = aaa ? aa :\n"
+ "? cc :\n"
+ " d;\n",
+ Style);
}
TEST_F(FormatTest, BreaksConditionalExpressionsAfterOperator) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1041,8 +1041,10 @@
//* not remove the 'lead' ContinuationIndentWidth
//* always un-indent by the operator when
//BreakBeforeTernaryOperators=true
- unsigned Indent =
- State.Stack.back().Indent - Style.ContinuationIndentWidth;
+ unsigned Indent = State.Stack.back().Indent;
+ if (Style.AlignOperands != FormatStyle::OAS_DontAlign) {
+Indent -= Style.ContinuationIndentWidth;
+ }
if (Style.BreakBeforeTernaryOperators &&
State.Stack.back().UnindentOperator)
Indent -= 2;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82199: [clang-format] restore indent in conditionals when AlignOperands is DontAlign
krasimir updated this revision to Diff 272966.
krasimir added a comment.
- try to update Phabricator diff
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82199/new/
https://reviews.llvm.org/D82199
Files:
clang/lib/Format/ContinuationIndenter.cpp
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6281,6 +6281,17 @@
" : bbb ? \n"
" : ;",
Style);
+
+ Style.AlignOperands = FormatStyle::OAS_DontAlign;
+ Style.BreakBeforeTernaryOperators = false;
+ // FIXME: Aligning the question marks is weird given DontAlign.
+ // Consider disabling this alignment in this case. Also check whether this
+ // will render the adjustment from https://reviews.llvm.org/D82199
+ // unnecessary.
+ verifyFormat("int x = aaa ? aa :\n"
+ "? cc :\n"
+ " d;\n",
+ Style);
}
TEST_F(FormatTest, BreaksConditionalExpressionsAfterOperator) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1041,8 +1041,10 @@
//* not remove the 'lead' ContinuationIndentWidth
//* always un-indent by the operator when
//BreakBeforeTernaryOperators=true
- unsigned Indent =
- State.Stack.back().Indent - Style.ContinuationIndentWidth;
+ unsigned Indent = State.Stack.back().Indent;
+ if (Style.AlignOperands != FormatStyle::OAS_DontAlign) {
+Indent -= Style.ContinuationIndentWidth;
+ }
if (Style.BreakBeforeTernaryOperators &&
State.Stack.back().UnindentOperator)
Indent -= 2;
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6281,6 +6281,17 @@
" : bbb ? \n"
" : ;",
Style);
+
+ Style.AlignOperands = FormatStyle::OAS_DontAlign;
+ Style.BreakBeforeTernaryOperators = false;
+ // FIXME: Aligning the question marks is weird given DontAlign.
+ // Consider disabling this alignment in this case. Also check whether this
+ // will render the adjustment from https://reviews.llvm.org/D82199
+ // unnecessary.
+ verifyFormat("int x = aaa ? aa :\n"
+ "? cc :\n"
+ " d;\n",
+ Style);
}
TEST_F(FormatTest, BreaksConditionalExpressionsAfterOperator) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1041,8 +1041,10 @@
//* not remove the 'lead' ContinuationIndentWidth
//* always un-indent by the operator when
//BreakBeforeTernaryOperators=true
- unsigned Indent =
- State.Stack.back().Indent - Style.ContinuationIndentWidth;
+ unsigned Indent = State.Stack.back().Indent;
+ if (Style.AlignOperands != FormatStyle::OAS_DontAlign) {
+Indent -= Style.ContinuationIndentWidth;
+ }
if (Style.BreakBeforeTernaryOperators &&
State.Stack.back().UnindentOperator)
Indent -= 2;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82199: [clang-format] restore indent in conditionals when AlignOperands is DontAlign
krasimir updated this revision to Diff 272964.
krasimir added a comment.
- refresh Phabricator diff
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82199/new/
https://reviews.llvm.org/D82199
Files:
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6284,6 +6284,10 @@
Style.AlignOperands = FormatStyle::OAS_DontAlign;
Style.BreakBeforeTernaryOperators = false;
+ // FIXME: Aligning the question marks is weird given DontAlign.
+ // Consider disabling this alignment in this case. Also check whether this
+ // will render the adjustment from https://reviews.llvm.org/D82199
+ // unnecessary.
verifyFormat("int x = aaa ? aa :\n"
"? cc :\n"
" d;\n",
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6284,6 +6284,10 @@
Style.AlignOperands = FormatStyle::OAS_DontAlign;
Style.BreakBeforeTernaryOperators = false;
+ // FIXME: Aligning the question marks is weird given DontAlign.
+ // Consider disabling this alignment in this case. Also check whether this
+ // will render the adjustment from https://reviews.llvm.org/D82199
+ // unnecessary.
verifyFormat("int x = aaa ? aa :\n"
"? cc :\n"
" d;\n",
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82199: [clang-format] restore indent in conditionals when AlignOperands is DontAlign
krasimir updated this revision to Diff 272957.
krasimir marked an inline comment as done.
krasimir added a comment.
- add a FIXME
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82199/new/
https://reviews.llvm.org/D82199
Files:
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6284,6 +6284,10 @@
Style.AlignOperands = FormatStyle::OAS_DontAlign;
Style.BreakBeforeTernaryOperators = false;
+ // FIXME: Aligning the question marks is weird given DontAlign.
+ // Consider disabling this alignment in this case. Also check whether this
+ // will render the adjustment from https://reviews.llvm.org/D82199
+ // unnecessary.
verifyFormat("int x = aaa ? aa :\n"
"? cc :\n"
" d;\n",
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6284,6 +6284,10 @@
Style.AlignOperands = FormatStyle::OAS_DontAlign;
Style.BreakBeforeTernaryOperators = false;
+ // FIXME: Aligning the question marks is weird given DontAlign.
+ // Consider disabling this alignment in this case. Also check whether this
+ // will render the adjustment from https://reviews.llvm.org/D82199
+ // unnecessary.
verifyFormat("int x = aaa ? aa :\n"
"? cc :\n"
" d;\n",
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82199: [clang-format] restore indent in conditionals when AlignOperands is DontAlign
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
This may not be the right long-term solution, but the current behavior is
pretty broken and this is cheap.
Comment at: clang/unittests/Format/FormatTest.cpp:6287
+ Style.BreakBeforeTernaryOperators = false;
+ verifyFormat("int x = aaa ? aa :\n"
+ "? cc :\n"
aligning the question marks here is a bit weird (given DontAlign) but that's
another patch.
If we disable the question-column behavior with dontalign, this patch will be
completely dead, right?
May want to add a FIXME to remove in that case.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82199/new/
https://reviews.llvm.org/D82199
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
