[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC332434: clang-format: tweak formatting of variable 
initialization blocks (authored by Typz, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D43290?vs=147002=147003#toc

Repository:
  rC Clang

https://reviews.llvm.org/D43290

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2311,6 +2311,8 @@
   if (Left.opensScope()) {
 if (Style.AlignAfterOpenBracket == FormatStyle::BAS_DontAlign)
   return 0;
+if (Left.is(tok::l_brace) && !Style.Cpp11BracedListStyle)
+  return 19;
 return Left.ParameterCount > 1 ? Style.PenaltyBreakBeforeFirstCallParameter
: 19;
   }
@@ -3048,6 +3050,9 @@
   if (Left.is(tok::equal) && !Right.isOneOf(tok::kw_default, tok::kw_delete) &&
   Line.Type == LT_VirtualFunctionDecl && Left.NestingLevel == 0)
 return false;
+  if (Left.is(tok::equal) && Right.is(tok::l_brace) &&
+  !Style.Cpp11BracedListStyle)
+return false;
   if (Left.is(tok::l_paren) && Left.is(TT_AttributeParen))
 return false;
   if (Left.is(tok::l_paren) && Left.Previous &&
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6708,6 +6708,15 @@
"};");
   verifyFormat("#define A {a, a},");
 
+  // Avoid breaking between equal sign and opening brace
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable =\n"
+   "{{\"a\", 0},\n"
+   " {\"b\", 1},\n"
+   " {\"c\", 2}};",
+   AvoidBreakingFirstArgument);
+
   // Binpacking only if there is no trailing comma
   verifyFormat("const Aa a = {aa, bb,\n"
"  cc, dd};",
@@ -6864,6 +6873,21 @@
   verifyFormat("vector foo = { ::SomeGlobalFunction() };", ExtraSpaces);
   verifyFormat("const struct A a = { .a = 1, .b = 2 };", ExtraSpaces);
   verifyFormat("const struct A a = { [0] = 1, [1] = 2 };", ExtraSpaces);
+
+  // Avoid breaking between initializer/equal sign and opening brace
+  ExtraSpaces.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "  { \"a\", 0 },\n"
+   "  { \"b\", 1 },\n"
+   "  { \"c\", 2 }\n"
+   "};",
+   ExtraSpaces);
+  verifyFormat("const std::unordered_map MyHashTable{\n"
+   "  { \"a\", 0 },\n"
+   "  { \"b\", 1 },\n"
+   "  { \"c\", 2 }\n"
+   "};",
+   ExtraSpaces);
 }
 
 TEST_F(FormatTest, FormatsBracedListsInColumnLayout) {


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2311,6 +2311,8 @@
   if (Left.opensScope()) {
 if (Style.AlignAfterOpenBracket == FormatStyle::BAS_DontAlign)
   return 0;
+if (Left.is(tok::l_brace) && !Style.Cpp11BracedListStyle)
+  return 19;
 return Left.ParameterCount > 1 ? Style.PenaltyBreakBeforeFirstCallParameter
: 19;
   }
@@ -3048,6 +3050,9 @@
   if (Left.is(tok::equal) && !Right.isOneOf(tok::kw_default, tok::kw_delete) &&
   Line.Type == LT_VirtualFunctionDecl && Left.NestingLevel == 0)
 return false;
+  if (Left.is(tok::equal) && Right.is(tok::l_brace) &&
+  !Style.Cpp11BracedListStyle)
+return false;
   if (Left.is(tok::l_paren) && Left.is(TT_AttributeParen))
 return false;
   if (Left.is(tok::l_paren) && Left.Previous &&
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6708,6 +6708,15 @@
"};");
   verifyFormat("#define A {a, a},");
 
+  // Avoid breaking between equal sign and opening brace
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable =\n"
+   "{{\"a\", 0},\n"
+   " {\"b\", 1},\n"
+   " {\"c\", 2}};",
+   

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 147002.
Typz marked 6 inline comments as done.
Typz added a comment.

Address review comment & rebase


Repository:
  rC Clang

https://reviews.llvm.org/D43290

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6708,6 +6708,15 @@
"};");
   verifyFormat("#define A {a, a},");
 
+  // Avoid breaking between equal sign and opening brace
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable =\n"
+   "{{\"a\", 0},\n"
+   " {\"b\", 1},\n"
+   " {\"c\", 2}};",
+   AvoidBreakingFirstArgument);
+
   // Binpacking only if there is no trailing comma
   verifyFormat("const Aa a = {aa, bb,\n"
"  cc, dd};",
@@ -6864,6 +6873,21 @@
   verifyFormat("vector foo = { ::SomeGlobalFunction() };", ExtraSpaces);
   verifyFormat("const struct A a = { .a = 1, .b = 2 };", ExtraSpaces);
   verifyFormat("const struct A a = { [0] = 1, [1] = 2 };", ExtraSpaces);
+
+  // Avoid breaking between initializer/equal sign and opening brace
+  ExtraSpaces.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "  { \"a\", 0 },\n"
+   "  { \"b\", 1 },\n"
+   "  { \"c\", 2 }\n"
+   "};",
+   ExtraSpaces);
+  verifyFormat("const std::unordered_map MyHashTable{\n"
+   "  { \"a\", 0 },\n"
+   "  { \"b\", 1 },\n"
+   "  { \"c\", 2 }\n"
+   "};",
+   ExtraSpaces);
 }
 
 TEST_F(FormatTest, FormatsBracedListsInColumnLayout) {
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2311,6 +2311,8 @@
   if (Left.opensScope()) {
 if (Style.AlignAfterOpenBracket == FormatStyle::BAS_DontAlign)
   return 0;
+if (Left.is(tok::l_brace) && !Style.Cpp11BracedListStyle)
+  return 19;
 return Left.ParameterCount > 1 ? Style.PenaltyBreakBeforeFirstCallParameter
: 19;
   }
@@ -3048,6 +3050,9 @@
   if (Left.is(tok::equal) && !Right.isOneOf(tok::kw_default, tok::kw_delete) &&
   Line.Type == LT_VirtualFunctionDecl && Left.NestingLevel == 0)
 return false;
+  if (Left.is(tok::equal) && Right.is(tok::l_brace) &&
+  !Style.Cpp11BracedListStyle)
+return false;
   if (Left.is(tok::l_paren) && Left.is(TT_AttributeParen))
 return false;
   if (Left.is(tok::l_paren) && Left.Previous &&


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6708,6 +6708,15 @@
"};");
   verifyFormat("#define A {a, a},");
 
+  // Avoid breaking between equal sign and opening brace
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable =\n"
+   "{{\"a\", 0},\n"
+   " {\"b\", 1},\n"
+   " {\"c\", 2}};",
+   AvoidBreakingFirstArgument);
+
   // Binpacking only if there is no trailing comma
   verifyFormat("const Aa a = {aa, bb,\n"
"  cc, dd};",
@@ -6864,6 +6873,21 @@
   verifyFormat("vector foo = { ::SomeGlobalFunction() };", ExtraSpaces);
   verifyFormat("const struct A a = { .a = 1, .b = 2 };", ExtraSpaces);
   verifyFormat("const struct A a = { [0] = 1, [1] = 2 };", ExtraSpaces);
+
+  // Avoid breaking between initializer/equal sign and opening brace
+  ExtraSpaces.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "  { \"a\", 0 },\n"
+   "  { \"b\", 1 },\n"
+   "  { \"c\", 2 }\n"
+   "};",
+   ExtraSpaces);
+  verifyFormat("const std::unordered_map MyHashTable{\n"
+   "  { \"a\", 0 },\n"
+   "  { \"b\", 1 },\n"
+   " 

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Generally looks good.




Comment at: lib/Format/TokenAnnotator.cpp:2183
   return 0;
+if (Left.Previous && Left.Previous->is(tok::equal) &&
+!Style.Cpp11BracedListStyle)

Typz wrote:
> djasper wrote:
> > Why is this necessary?
> Otherwise the `PenaltyBreakBeforeFirstCallParameter` is used, which is not 
> consistent with the concept of !Cpp11BraceListStyle (e.g. consider this is an 
> initializer), and gives the following format, which is certainly not the 
> expected result:
> 
>   const std::unordered_map
>   MyHashTable = { { \"a\", 0 },
>   { \"b\", 1 },
>   { \"c\", 2 } };
> 
> (again, the issue only happens when `PenaltyBreakBeforeFirstCallParameter` is 
> increased, e.g. 200 in my case. The default value is 19, so formatting is not 
> affected)
I think PenaltyBreakBeforeFirstCallParameter should not be applied with 
!Cpp11BracedListStyle whenever a brace is found. Cpp11BracedList style means 
that braces are to be treated like function calls, but without it, this doesn't 
make sense. I think this is in some ways better than looking for the "= {".



Comment at: unittests/Format/FormatTest.cpp:6655
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable =\n"

Typz wrote:
> djasper wrote:
> > How does this penalty influence the test?
> Because breaking after the opening brace is affected by the 
> `PenaltyBreakBeforeFirstCallParameter` : this is an init braced-list, but it 
> is considered as any function.
> 
> Specifically, my patch needs (see prev. comment) to change the penalty for 
> breaking after the brace, but this applies only when 
> `Cpp11BracedListStyle=false`, as per your earlier comment. So this test just 
> verify that the behavior is indeed not affected.
I see.


Repository:
  rC Clang

https://reviews.llvm.org/D43290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

@djasper : ping?


Repository:
  rC Clang

https://reviews.llvm.org/D43290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


Repository:
  rC Clang

https://reviews.llvm.org/D43290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


Repository:
  rC Clang

https://reviews.llvm.org/D43290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


Repository:
  rC Clang

https://reviews.llvm.org/D43290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:2183
   return 0;
+if (Left.Previous && Left.Previous->is(tok::equal) &&
+!Style.Cpp11BracedListStyle)

djasper wrote:
> Why is this necessary?
Otherwise the `PenaltyBreakBeforeFirstCallParameter` is used, which is not 
consistent with the concept of !Cpp11BraceListStyle (e.g. consider this is an 
initializer), and gives the following format, which is certainly not the 
expected result:

  const std::unordered_map
  MyHashTable = { { \"a\", 0 },
  { \"b\", 1 },
  { \"c\", 2 } };

(again, the issue only happens when `PenaltyBreakBeforeFirstCallParameter` is 
increased, e.g. 200 in my case. The default value is 19, so formatting is not 
affected)



Comment at: unittests/Format/FormatTest.cpp:6655
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable =\n"

djasper wrote:
> How does this penalty influence the test?
Because breaking after the opening brace is affected by the 
`PenaltyBreakBeforeFirstCallParameter` : this is an init braced-list, but it is 
considered as any function.

Specifically, my patch needs (see prev. comment) to change the penalty for 
breaking after the brace, but this applies only when 
`Cpp11BracedListStyle=false`, as per your earlier comment. So this test just 
verify that the behavior is indeed not affected.


Repository:
  rC Clang

https://reviews.llvm.org/D43290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:2183
   return 0;
+if (Left.Previous && Left.Previous->is(tok::equal) &&
+!Style.Cpp11BracedListStyle)

Why is this necessary?



Comment at: unittests/Format/FormatTest.cpp:6655
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable =\n"

How does this penalty influence the test?


Repository:
  rC Clang

https://reviews.llvm.org/D43290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 136081.
Typz added a comment.

Prevent breaking between = and { when Cpp11BracedListStyle=false.


Repository:
  rC Clang

https://reviews.llvm.org/D43290

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6650,6 +6650,15 @@
"};");
   verifyFormat("#define A {a, a},");
 
+  // Avoid breaking between equal sign and opening brace
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable =\n"
+   "{{\"a\", 0},\n"
+   " {\"b\", 1},\n"
+   " {\"c\", 2}};",
+   AvoidBreakingFirstArgument);
+
   // Binpacking only if there is no trailing comma
   verifyFormat("const Aa a = {aa, bb,\n"
"  cc, dd};",
@@ -6806,6 +6815,15 @@
   verifyFormat("vector foo = { ::SomeGlobalFunction() };", ExtraSpaces);
   verifyFormat("const struct A a = { .a = 1, .b = 2 };", ExtraSpaces);
   verifyFormat("const struct A a = { [0] = 1, [1] = 2 };", ExtraSpaces);
+
+  // Avoid breaking between equal sign and opening brace
+  ExtraSpaces.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "  { \"a\", 0 },\n"
+   "  { \"b\", 1 },\n"
+   "  { \"c\", 2 }\n"
+   "};",
+   ExtraSpaces);
 }
 
 TEST_F(FormatTest, FormatsBracedListsInColumnLayout) {
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2180,6 +2180,9 @@
   if (Left.opensScope()) {
 if (Style.AlignAfterOpenBracket == FormatStyle::BAS_DontAlign)
   return 0;
+if (Left.Previous && Left.Previous->is(tok::equal) &&
+!Style.Cpp11BracedListStyle)
+  return 19;
 return Left.ParameterCount > 1 ? Style.PenaltyBreakBeforeFirstCallParameter
: 19;
   }
@@ -2863,6 +2866,9 @@
   if (Left.is(tok::equal) && !Right.isOneOf(tok::kw_default, tok::kw_delete) &&
   Line.Type == LT_VirtualFunctionDecl && Left.NestingLevel == 0)
 return false;
+  if (Left.is(tok::equal) && Right.is(tok::l_brace) &&
+  !Style.Cpp11BracedListStyle)
+return false;
   if (Left.is(tok::l_paren) && Left.is(TT_AttributeParen))
 return false;
   if (Left.is(tok::l_paren) && Left.Previous &&


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6650,6 +6650,15 @@
"};");
   verifyFormat("#define A {a, a},");
 
+  // Avoid breaking between equal sign and opening brace
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable =\n"
+   "{{\"a\", 0},\n"
+   " {\"b\", 1},\n"
+   " {\"c\", 2}};",
+   AvoidBreakingFirstArgument);
+
   // Binpacking only if there is no trailing comma
   verifyFormat("const Aa a = {aa, bb,\n"
"  cc, dd};",
@@ -6806,6 +6815,15 @@
   verifyFormat("vector foo = { ::SomeGlobalFunction() };", ExtraSpaces);
   verifyFormat("const struct A a = { .a = 1, .b = 2 };", ExtraSpaces);
   verifyFormat("const struct A a = { [0] = 1, [1] = 2 };", ExtraSpaces);
+
+  // Avoid breaking between equal sign and opening brace
+  ExtraSpaces.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "  { \"a\", 0 },\n"
+   "  { \"b\", 1 },\n"
+   "  { \"c\", 2 }\n"
+   "};",
+   ExtraSpaces);
 }
 
 TEST_F(FormatTest, FormatsBracedListsInColumnLayout) {
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2180,6 +2180,9 @@
   if (Left.opensScope()) {
 if (Style.AlignAfterOpenBracket == FormatStyle::BAS_DontAlign)
   return 0;
+if (Left.Previous && Left.Previous->is(tok::equal) &&
+!Style.Cpp11BracedListStyle)
+  return 19;
 return 

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

In https://reviews.llvm.org/D43290#1020537, @Typz wrote:

> >> Tweaking the penalty handling would still be needed in any case, since the 
> >> problem happens also when Cpp11BracedListStyle is true (though the effect 
> >> is more subtle)
> > 
> > I don't understand this. Which style do you actually care about? With 
> > Cpp11BracedListStyle=true or =false?
>
> I use the `Cpp11BracedListStyle=false` style.
>  However, I think the current behavior is wrong also when 
> `Cpp11BracedListStyle=true`. Here the behavior in this case:
>
> Before :
>
>   const std::unordered_map Something::MyHashTable =
>   {{ "a", 0 },
>{ "b", 1 },
>{ "c", 2 }};
>   
>
> After :
>
>   const std::unordered_set Something::MyUnorderedSet = {
>   { "a", 0 },
>   { "b", 1 },
>   { "c", 2 }};


"Before" is the desired behavior. I don't know whether other people have 
extensively analyzed how to format all the various C++11 braced lists, but we 
have at Google and think this is good. It is formalized here:
https://google.github.io/styleguide/cppguide.html#Braced_Initializer_List_Format

We prefer breaking before "{" at the higher syntactic level or essentially 
before the imaginary function call in place of the braced list.

>>> Generally, I think it is better to just rely on penalties, since it gives a 
>>> way to compare and weight each solution. Then each style can decide what 
>>> should break first: e.g. a style may also have a lower 
>>> `PenaltyBreakAssignment`, thus wrapping after the equal sign would be 
>>> expected...
>> 
>> And with my reasoning, I think exactly the opposite. Penalties are nice, but 
>> if the behavior is generally unwanted, than it's very hard to predict in 
>> which situations it might still occur.
> 
> Yet on the other hand enforcing too much can lead to cases where the 
> optimizer fails to find a solution, and ends up simply not formatting the 
> line: which is fine is the code is already formatted (but if there were the 
> case we would not need the tool at all :-) )
>  The beauty of penalties is that one can tweak the different penalties so 
> that the behavior is "generally" what would be expected: definitely not 
> predictable, but then there are always cases where the "regular" style does 
> not work, and fallback solutions must be used... (for exemple, I would prefer 
> never to never break after an assignment : yet sometimes it is needed...)
> 
> I can change the patch to disallow breaking, but this would be a slightly 
> more risky change, with possibly more side-effects; and i think that would 
> not solve the issue when `Cpp11BracedListStyle=true`.

It is the better call here. I understand that people that set 
Cpp11BracedListStyle to false want this behavior and I think it'll always be a 
surprise when clang-format breaks before the "{". The chance of not coming up 
with a formatting because of this is somewhat slim. It only adds an additional 
two characters (we would also not break before the "="). It'd be really weird 
to only have these exact two line length have a special behavior (slightly 
shorter - everything fits on one line, slightly longer - a split between type 
name and variable is necessary).

There is no issue for Cpp11BracedListStyle=true, the behavior is as desired.


Repository:
  rC Clang

https://reviews.llvm.org/D43290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

>> Tweaking the penalty handling would still be needed in any case, since the 
>> problem happens also when Cpp11BracedListStyle is true (though the effect is 
>> more subtle)
> 
> I don't understand this. Which style do you actually care about? With 
> Cpp11BracedListStyle=true or =false?

I use the `Cpp11BracedListStyle=false` style.
However, I think the current behavior is wrong also when 
`Cpp11BracedListStyle=true`. Here the behavior in this case:

Before :

  const std::unordered_map Something::MyHashTable =
  {{ "a", 0 },
   { "b", 1 },
   { "c", 2 }};

After :

  const std::unordered_set Something::MyUnorderedSet = {
  { "a", 0 },
  { "b", 1 },
  { "c", 2 }};

>> Generally, I think it is better to just rely on penalties, since it gives a 
>> way to compare and weight each solution. Then each style can decide what 
>> should break first: e.g. a style may also have a lower 
>> `PenaltyBreakAssignment`, thus wrapping after the equal sign would be 
>> expected...
> 
> And with my reasoning, I think exactly the opposite. Penalties are nice, but 
> if the behavior is generally unwanted, than it's very hard to predict in 
> which situations it might still occur.

Yet on the other hand enforcing too much can lead to cases where the optimizer 
fails to find a solution, and ends up simply not formatting the line: which is 
fine is the code is already formatted (but if there were the case we would not 
need the tool at all :-) )
The beauty of penalties is that one can tweak the different penalties so that 
the behavior is "generally" what would be expected: definitely not predictable, 
but then there are always cases where the "regular" style does not work, and 
fallback solutions must be used... (for exemple, I would prefer never to never 
break after an assignment : yet sometimes it is needed...)

I can change the patch to disallow breaking, but this would be a slightly more 
risky change, with possibly more side-effects; and i think that would not solve 
the issue when `Cpp11BracedListStyle=true`.


Repository:
  rC Clang

https://reviews.llvm.org/D43290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

In https://reviews.llvm.org/D43290#1008647, @Typz wrote:

> Tweaking the penalty handling would still be needed in any case, since the 
> problem happens also when Cpp11BracedListStyle is true (though the effect is 
> more subtle)


I don't understand this. Which style do you actually care about? With 
Cpp11BracedListStyle=true or =false?

> Generally, I think it is better to just rely on penalties, since it gives a 
> way to compare and weight each solution. Then each style can decide what 
> should break first: e.g. a style may also have a lower 
> `PenaltyBreakAssignment`, thus wrapping after the equal sign would be 
> expected...

And with my reasoning, I think exactly the opposite. Penalties are nice, but if 
the behavior is generally unwanted, than it's very hard to predict in which 
situations it might still occur.


Repository:
  rC Clang

https://reviews.llvm.org/D43290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 134411.
Typz marked an inline comment as done.
Typz added a comment.

Address review comments.


Repository:
  rC Clang

https://reviews.llvm.org/D43290

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6650,6 +6650,15 @@
"};");
   verifyFormat("#define A {a, a},");
 
+  // Avoid breaking between equal sign and opening brace
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "{\"a\", 0},\n"
+   "{\"b\", 1},\n"
+   "{\"c\", 2}};",
+   AvoidBreakingFirstArgument);
+
   // Binpacking only if there is no trailing comma
   verifyFormat("const Aa a = {aa, bb,\n"
"  cc, dd};",
@@ -6806,6 +6815,15 @@
   verifyFormat("vector foo = { ::SomeGlobalFunction() };", ExtraSpaces);
   verifyFormat("const struct A a = { .a = 1, .b = 2 };", ExtraSpaces);
   verifyFormat("const struct A a = { [0] = 1, [1] = 2 };", ExtraSpaces);
+
+  // Avoid breaking between equal sign and opening brace
+  ExtraSpaces.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "  { \"a\", 0 },\n"
+   "  { \"b\", 1 },\n"
+   "  { \"c\", 2 }\n"
+   "};",
+   ExtraSpaces);
 }
 
 TEST_F(FormatTest, FormatsBracedListsInColumnLayout) {
@@ -6867,6 +6885,14 @@
" 3, cc};",
getLLVMStyleWithColumns(60));
 
+  // Do not break between equal and opening brace.
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyleWithColumns(43);
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  AvoidBreakingFirstArgument.ColumnLimit = 43;
+  verifyFormat("vector aa = {\n"
+   "1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1};",
+   AvoidBreakingFirstArgument);
+
   // Trailing commas.
   verifyFormat("vector x = {\n"
"1, 1, 1, 1, 1, 1, 1, 1,\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2180,6 +2180,8 @@
   if (Left.opensScope()) {
 if (Style.AlignAfterOpenBracket == FormatStyle::BAS_DontAlign)
   return 0;
+if (Left.Previous && Left.Previous->is(tok::equal))
+  return 19;
 return Left.ParameterCount > 1 ? Style.PenaltyBreakBeforeFirstCallParameter
: 19;
   }


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6650,6 +6650,15 @@
"};");
   verifyFormat("#define A {a, a},");
 
+  // Avoid breaking between equal sign and opening brace
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "{\"a\", 0},\n"
+   "{\"b\", 1},\n"
+   "{\"c\", 2}};",
+   AvoidBreakingFirstArgument);
+
   // Binpacking only if there is no trailing comma
   verifyFormat("const Aa a = {aa, bb,\n"
"  cc, dd};",
@@ -6806,6 +6815,15 @@
   verifyFormat("vector foo = { ::SomeGlobalFunction() };", ExtraSpaces);
   verifyFormat("const struct A a = { .a = 1, .b = 2 };", ExtraSpaces);
   verifyFormat("const struct A a = { [0] = 1, [1] = 2 };", ExtraSpaces);
+
+  // Avoid breaking between equal sign and opening brace
+  ExtraSpaces.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "  { \"a\", 0 },\n"
+   "  { \"b\", 1 },\n"
+   "  { \"c\", 2 }\n"
+   "};",
+   ExtraSpaces);
 }
 
 TEST_F(FormatTest, FormatsBracedListsInColumnLayout) {
@@ -6867,6 +6885,14 @@
" 3, cc};",
getLLVMStyleWithColumns(60));
 
+  // Do not break between equal and opening brace.
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyleWithColumns(43);

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

Tweaking the penalty handling would still be needed in any case, since the 
problem happens also when Cpp11BracedListStyle is true (though the effect is 
more subtle)

Generally, I think it is better to just rely on penalties, since it gives a way 
to compare and weight each solution. Then each style can decide what should 
break first: e.g. a style may also have a lower `PenaltyBreakAssignment`, thus 
wrapping after the equal sign would be expected...


Repository:
  rC Clang

https://reviews.llvm.org/D43290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

What you are doing makes sense to me. My only hesitation is whether we should 
maybe completely disallow breaking between = and { when Cpp11BracedListStyle is 
false instead of solving this via penalties. Specifically,

 | 80 column limit
  SomethingReallyLong =
  { { a, b, c },
{ a, b, c } };

Might not be as good as

 | 80 column limit
  SomethingReallyLong<
  SomethingReallyLong> = {
{ a, b, c },
{ a, b, c }
  };

in this mode.




Comment at: unittests/Format/FormatTest.cpp:6889
+  // Do not break between equal and opening brace.
+  FormatStyle avoidBreakingFirstArgument = getLLVMStyleWithColumns(43);
+  avoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;

Upper camel case for variable names according to LLVM style.


Repository:
  rC Clang

https://reviews.llvm.org/D43290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz created this revision.
Typz added reviewers: krasimir, djasper, klimek.

This patch changes the behavior of PenaltyBreakBeforeFirstCallParameter
so that is does not apply when the brace comes after an assignment.

This way, variable initialization is wrapped more like an initializer
than like a function call, which seems more consistent with user
expectations.

With PenaltyBreakBeforeFirstCallParameter=200, this gives the following
code: (with Cpp11BracedListStyle=false)

Before :

  const std::unordered_map Something::MyHashTable =
  { { "a", 0 },
{ "b", 1 },
{ "c", 2 } };

After :

  const std::unordered_set Something::MyUnorderedSet = {
{ "a", 0 },
{ "b", 1 },
{ "c", 2 }
  };


Repository:
  rC Clang

https://reviews.llvm.org/D43290

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6650,6 +6650,15 @@
"};");
   verifyFormat("#define A {a, a},");
 
+  // Avoid breaking between equal sign and opening brace
+  FormatStyle avoidBreakingFirstArgument = getLLVMStyle();
+  avoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "{\"a\", 0},\n"
+   "{\"b\", 1},\n"
+   "{\"c\", 2}};",
+   avoidBreakingFirstArgument);
+
   // Binpacking only if there is no trailing comma
   verifyFormat("const Aa a = {aa, bb,\n"
"  cc, dd};",
@@ -6806,6 +6815,15 @@
   verifyFormat("vector foo = { ::SomeGlobalFunction() };", ExtraSpaces);
   verifyFormat("const struct A a = { .a = 1, .b = 2 };", ExtraSpaces);
   verifyFormat("const struct A a = { [0] = 1, [1] = 2 };", ExtraSpaces);
+
+  // Avoid breaking between equal sign and opening brace
+  ExtraSpaces.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "  { \"a\", 0 },\n"
+   "  { \"b\", 1 },\n"
+   "  { \"c\", 2 }\n"
+   "};",
+   ExtraSpaces);
 }
 
 TEST_F(FormatTest, FormatsBracedListsInColumnLayout) {
@@ -6867,6 +6885,14 @@
" 3, cc};",
getLLVMStyleWithColumns(60));
 
+  // Do not break between equal and opening brace.
+  FormatStyle avoidBreakingFirstArgument = getLLVMStyleWithColumns(43);
+  avoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  avoidBreakingFirstArgument.ColumnLimit = 43;
+  verifyFormat("vector aa = {\n"
+   "1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1};",
+   avoidBreakingFirstArgument);
+
   // Trailing commas.
   verifyFormat("vector x = {\n"
"1, 1, 1, 1, 1, 1, 1, 1,\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2180,6 +2180,8 @@
   if (Left.opensScope()) {
 if (Style.AlignAfterOpenBracket == FormatStyle::BAS_DontAlign)
   return 0;
+if (Left.Previous && Left.Previous->is(tok::equal))
+  return 19;
 return Left.ParameterCount > 1 ? Style.PenaltyBreakBeforeFirstCallParameter
: 19;
   }


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6650,6 +6650,15 @@
"};");
   verifyFormat("#define A {a, a},");
 
+  // Avoid breaking between equal sign and opening brace
+  FormatStyle avoidBreakingFirstArgument = getLLVMStyle();
+  avoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable = {\n"
+   "{\"a\", 0},\n"
+   "{\"b\", 1},\n"
+   "{\"c\", 2}};",
+   avoidBreakingFirstArgument);
+
   // Binpacking only if there is no trailing comma
   verifyFormat("const Aa a = {aa, bb,\n"
"  cc, dd};",
@@ -6806,6 +6815,15 @@
   verifyFormat("vector foo = { ::SomeGlobalFunction() };", ExtraSpaces);
   verifyFormat("const struct A a = { .a = 1, .b = 2 };", ExtraSpaces);
   verifyFormat("const struct A a = { [0] = 1, [1] = 2 };", ExtraSpaces);
+
+