[PATCH] D14104: clang-format: Add an additional value to AlignAfterOpenBracket: AlwaysBreak.

[Martin Probst via cfe-commits]

mprobst added a comment.

Nice, great to see this.


================
Comment at: lib/Format/ContinuationIndenter.cpp:330
@@ -329,3 +329,3 @@
 
-  if (Style.AlignAfterOpenBracket && Previous.opensScope() &&
-      Previous.isNot(TT_ObjCMethodExpr) &&
+  if (Style.AlignAfterOpenBracket == FormatStyle::BAS_AlwaysBreak &&
+      Previous.is(tok::l_paren) && State.Column > getNewLineColumn(State) &&
----------------
Nice.

Maybe add a comment along the lines off `// In "AlwaysBreak" mode, enforce wrapping directly after the parenthesis by disallowing any further line breaks if there is no line break after the opening parenthesis` or so?

================
Comment at: lib/Format/ContinuationIndenter.cpp:882
@@ -876,1 +881,3 @@
                                         State.Stack.back().NestedBlockIndent);
+  bool NoLineBreak = State.Stack.back().NoLineBreak ||
+                     (Current.is(TT_TemplateOpener) &&
----------------
Maybe add a comment on how this works? It's not clear to me. Is this change (with the assignment below) to allow breaks in block-style literals?

================
Comment at: lib/Format/ContinuationIndenter.cpp:983
@@ -976,2 +982,3 @@
       State.Stack.back().LastSpace, /*AvoidBinPacking=*/true,
-      State.Stack.back().NoLineBreak));
+      false));
+      // State.Stack.back().NoLineBreak));
----------------
Nit: add a comment on what the parameter is? `/*NoLineBreak*/ false`? And maybe explain why code below needs this to be false?

================
Comment at: lib/Format/ContinuationIndenter.cpp:984
@@ -978,1 +983,3 @@
+      false));
+      // State.Stack.back().NoLineBreak));
   State.Stack.back().NestedBlockIndent = NestedBlockIndent;
----------------
Nit: leftover comment?

================
Comment at: lib/Format/TokenAnnotator.cpp:393
@@ -392,6 +392,3 @@
   void updateParameterCount(FormatToken *Left, FormatToken *Current) {
-    if (Current->is(TT_LambdaLSquare) ||
-        (Current->is(tok::caret) && Current->is(TT_UnaryOperator)) ||
-        (Style.Language == FormatStyle::LK_JavaScript &&
-         Current->is(Keywords.kw_function))) {
+    if (Current->is(tok::l_brace) && !Current->is(TT_DictLiteral))
       ++Left->BlockParameterCount;
----------------
I don't understand why this changes how we count block parameters - is this an unrelated cleanup?

================
Comment at: unittests/Format/FormatTestJS.cpp:296
@@ -294,8 +295,3 @@
                "]);");
-  verifyFormat("var someVariable = SomeFuntion(aaaa,\n"
-               "                               [\n"
-               "                                 aaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
-               "                                 bbbbbbbbbbbbbbbbbbbbbbbbbbb,\n"
-               "                                 ccccccccccccccccccccccccccc\n"
-               "                               ],\n"
-               "                               aaaa);");
+  verifyFormat("var someVariable = SomeFuntion(\n"
+               "    aaaa,\n"
----------------
nit: typo in `SomeFunction`

================
Comment at: unittests/Format/FormatTestJS.cpp:325
@@ -322,9 +324,3 @@
                "};");
-  EXPECT_EQ("abc = xyz ?\n"
-            "          function() {\n"
-            "            return 1;\n"
-            "          } :\n"
-            "          function() {\n"
-            "            return -1;\n"
-            "          };",
-            format("abc=xyz?function(){return 1;}:function(){return -1;};"));
+  verifyFormat("abc = xyz ? function() {\n"
+               "  return 1;\n"
----------------
This formatting is ok, I guess (and hopefully not too common in the first place), but I find it somewhat surprising that this is a side effect of this change. Can you explain?


http://reviews.llvm.org/D14104