[PATCH] D148131: Avoid unnecessarily aggressive line-breaking when using "LambdaBodyIndentation: OuterScope" with argument bin-packing.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 marked 2 inline comments as done.
jp4a50 added a comment.

Thanks all. All comments addressed now. Please merge for me once ready.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148131/new/

https://reviews.llvm.org/D148131

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

[PATCH] D148131: Avoid unnecessarily aggressive line-breaking when using "LambdaBodyIndentation: OuterScope" with argument bin-packing.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 556262.
jp4a50 added a comment.

Minor review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148131/new/

https://reviews.llvm.org/D148131

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
@@ -22524,8 +22524,7 @@
"  []() -> auto {\n"
"int b = 32;\n"
"return 3;\n"
-   "  },\n"
-   "  foo, bar)\n"
+   "  }, foo, bar)\n"
"  .foo();\n"
"}",
Style);
@@ -22551,32 +22550,12 @@
"  })));\n"
"}",
Style);
-  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
-  verifyFormat("void foo() {\n"
-   "  aFunction(\n"
-   "  1, b(c(\n"
-   " [](d) -> Foo {\n"
-   "auto f = e(d);\n"
-   "return f;\n"
-   "  },\n"
-   " foo, Bar{},\n"
-   " [] {\n"
-   "auto g = h();\n"
-   "return g;\n"
-   "  },\n"
-   " baz)));\n"
-   "}",
-   Style);
   verifyFormat("void foo() {\n"
"  aFunction(1, b(c(foo, Bar{}, baz, [](d) -> Foo {\n"
-   "auto f = e(\n"
-   "foo,\n"
-   "[&] {\n"
+   "auto f = e(foo, [&] {\n"
"  auto g = h();\n"
"  return g;\n"
-   "},\n"
-   "qux,\n"
-   "[&] -> Bar {\n"
+   "}, qux, [&] -> Bar {\n"
"  auto i = j();\n"
"  return i;\n"
"});\n"
@@ -22584,28 +22563,77 @@
"  })));\n"
"}",
Style);
-  verifyFormat("Namespace::Foo::Foo(\n"
-   "LongClassName bar, AnotherLongClassName baz)\n"
+  verifyFormat("Namespace::Foo::Foo(LongClassName bar,\n"
+   "AnotherLongClassName baz)\n"
": baz{baz}, func{[&] {\n"
"  auto qux = bar;\n"
"  return aFunkyFunctionCall(qux);\n"
"}} {}",
Style);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+#if 0
+  // FIXME: As long as all the non-lambda arguments fit on a single line, AlwaysBreak
+  // doesn't force an initial line break, even if lambdas span multiple lines.
+  // The following test should pass, but fails at the time of writing.
+  verifyFormat("void foo() {\n"
+   "  aFunction(\n"
+   "  [](d) -> Foo {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  }, foo, Bar{}, [] {\n"
+   "auto g = h();\n"
+   "return g;\n"
+   "  }, baz);\n"
+   "}",
+   Style);
+#endif
+  // A long non-lambda argument forces arguments to span multiple lines and thus
+  // forces an initial line break when using AlwaysBreak.
+  verifyFormat("void foo() {\n"
+   "  aFunction(\n"
+   "  1,\n"
+   "  [](d) -> Foo {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  }, foo, Bar{},\n"
+   "  [] {\n"
+   "auto g = h();\n"
+   "return g;\n"
+   "  }, baz,\n"
+   "  qx);\n"
+   "}",
+   Style);
+  Style.BinPackArguments = false;
+  verifyFormat("void foo() {\n"
+   "  aFunction(\n"
+   "  1,\n"
+   "  [](d) -> Foo {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  },\n"
+   "  foo,\n"
+   "  Bar{},\n"
+   "  [] {\n"
+   "auto g = h();\n"
+   "return g;\n"
+   "  },\n"
+   "  baz,\n"
+   "  qx);\n"
+   "}",
+   Style);
+  Style.BinPackArguments = true;
   Style.BreakBeforeBraces = FormatStyle::BS_Custom;
   Style.BraceWrapping.BeforeLambdaBody = true;
   verifyFormat("void foo() {\n"
"  aFunction(\n"
-   "  1, b(c(foo, Bar{}, baz,\n"
-   " [](d) -> Foo\n"
+   "  1, b(c(foo, Bar{}, baz, [](d) -> Foo\n"
"  {\n"
"auto f = e(\n"
   

[PATCH] D148131: Avoid unnecessarily aggressive line-breaking when using "LambdaBodyIndentation: OuterScope" with argument bin-packing.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

In D148131#4628719 , 
@HazardyKnusperkeks wrote:

> But please wait for other opinions.

OK sure. @owenpan, any thoughts?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148131/new/

https://reviews.llvm.org/D148131

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

[PATCH] D148131: Avoid unnecessarily aggressive line-breaking when using "LambdaBodyIndentation: OuterScope" with argument bin-packing.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 marked 6 inline comments as done.
jp4a50 added a comment.

All comments addressed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148131/new/

https://reviews.llvm.org/D148131

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

[PATCH] D148131: Avoid unnecessarily aggressive line-breaking when using "LambdaBodyIndentation: OuterScope" with argument bin-packing.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 554704.
jp4a50 added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148131/new/

https://reviews.llvm.org/D148131

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
@@ -22737,8 +22737,7 @@
"  []() -> auto {\n"
"int b = 32;\n"
"return 3;\n"
-   "  },\n"
-   "  foo, bar)\n"
+   "  }, foo, bar)\n"
"  .foo();\n"
"}",
Style);
@@ -22764,32 +22763,12 @@
"  })));\n"
"}",
Style);
-  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
-  verifyFormat("void foo() {\n"
-   "  aFunction(\n"
-   "  1, b(c(\n"
-   " [](d) -> Foo {\n"
-   "auto f = e(d);\n"
-   "return f;\n"
-   "  },\n"
-   " foo, Bar{},\n"
-   " [] {\n"
-   "auto g = h();\n"
-   "return g;\n"
-   "  },\n"
-   " baz)));\n"
-   "}",
-   Style);
   verifyFormat("void foo() {\n"
"  aFunction(1, b(c(foo, Bar{}, baz, [](d) -> Foo {\n"
-   "auto f = e(\n"
-   "foo,\n"
-   "[&] {\n"
+   "auto f = e(foo, [&] {\n"
"  auto g = h();\n"
"  return g;\n"
-   "},\n"
-   "qux,\n"
-   "[&] -> Bar {\n"
+   "}, qux, [&] -> Bar {\n"
"  auto i = j();\n"
"  return i;\n"
"});\n"
@@ -22797,28 +22776,74 @@
"  })));\n"
"}",
Style);
-  verifyFormat("Namespace::Foo::Foo(\n"
-   "LongClassName bar, AnotherLongClassName baz)\n"
+  verifyFormat("Namespace::Foo::Foo(LongClassName bar,\n"
+   "AnotherLongClassName baz)\n"
": baz{baz}, func{[&] {\n"
"  auto qux = bar;\n"
"  return aFunkyFunctionCall(qux);\n"
"}} {}",
Style);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  // As long as all the non-lambda arguments fit on a single line, AlwaysBreak
+  // doesn't force an initial line break, even if lambdas span multiple lines.
+  // This should probably be considered a bug.
+  verifyFormat("void foo() {\n"
+   "  aFunction([](d) -> Foo {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  }, foo, Bar{}, [] {\n"
+   "auto g = h();\n"
+   "return g;\n"
+   "  }, baz);\n"
+   "}",
+   Style);
+  // A long non-lambda argument forces arguments to span multiple lines and thus
+  // forces an initial line break when using AlwaysBreak.
+  verifyFormat("void foo() {\n"
+   "  aFunction(\n"
+   "  1,\n"
+   "  [](d) -> Foo {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  }, foo, Bar{},\n"
+   "  [] {\n"
+   "auto g = h();\n"
+   "return g;\n"
+   "  }, baz,\n"
+   "  qx);\n"
+   "}",
+   Style);
+  Style.BinPackArguments = false;
+  verifyFormat("void foo() {\n"
+   "  aFunction(\n"
+   "  1,\n"
+   "  [](d) -> Foo {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  },\n"
+   "  foo,\n"
+   "  Bar{},\n"
+   "  [] {\n"
+   "auto g = h();\n"
+   "return g;\n"
+   "  },\n"
+   "  baz,\n"
+   "  qx);\n"
+   "}",
+   Style);
+  Style.BinPackArguments = true;
   Style.BreakBeforeBraces = FormatStyle::BS_Custom;
   Style.BraceWrapping.BeforeLambdaBody = true;
   verifyFormat("void foo() {\n"
"  aFunction(\n"
-   "  1, b(c(foo, Bar{}, baz,\n"
-   " [](d) -> Foo\n"
+   "  1, b(c(foo, Bar{}, baz, [](d) -> Foo\n"
"  {\n"
"auto f = e(\n"
"[&]\n"
"{\n"
"  auto 

[PATCH] D148131: Avoid unnecessarily aggressive line-breaking when using "LambdaBodyIndentation: OuterScope" with argument bin-packing.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

Hello @owenpan , @HazardyKnusperkeks . Now that 
https://reviews.llvm.org/D156259 is merged, please can you take a look at this 
change?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148131/new/

https://reviews.llvm.org/D148131

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

[PATCH] D156259: [clang-format] Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 marked an inline comment as done.
jp4a50 added a comment.

@owenpan right you are! Missed those somehow. Made a further two changes. Hope 
I haven't missed anything else!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156259/new/

https://reviews.llvm.org/D156259

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

[PATCH] D156259: [clang-format] Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 552701.
jp4a50 added a comment.

Minor refactoring.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156259/new/

https://reviews.llvm.org/D156259

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22177,8 +22177,25 @@
"  }\n"
"};");
 
-  // Multiple lambdas in the same parentheses change indentation rules. These
-  // lambdas are forced to start on new lines.
+  // Lambdas that fit on a single line within an argument list are not forced
+  // onto new lines.
+  verifyFormat("SomeFunction([] {});");
+  verifyFormat("SomeFunction(0, [] {});");
+  verifyFormat("SomeFunction([] {}, 0);");
+  verifyFormat("SomeFunction(0, [] {}, 0);");
+  verifyFormat("SomeFunction([] { return 0; }, 0);");
+  verifyFormat("SomeFunction(a, [] { return 0; }, b);");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; });");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  verifyFormat("auto lng =\n"
+   "SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  // Exceeded column limit. We need to break.
+  verifyFormat("auto lngName = SomeFunction(\n"
+   "[] { return anotherLooonngName; }, [] { "
+   "return 0; }, b);");
+
+  // Multiple multi-line lambdas in the same parentheses change indentation
+  // rules. These lambdas are always forced to start on new lines.
   verifyFormat("SomeFunction(\n"
"[]() {\n"
"  //\n"
@@ -22187,7 +22204,7 @@
"  //\n"
"});");
 
-  // A lambda passed as arg0 is always pushed to the next line.
+  // A multi-line lambda passed as arg0 is always pushed to the next line.
   verifyFormat("SomeFunction(\n"
"[this] {\n"
"  //\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -5203,30 +5203,6 @@
   return true;
   }
 
-  // Deal with lambda arguments in C++ - we want consistent line breaks whether
-  // they happen to be at arg0, arg1 or argN. The selection is a bit nuanced
-  // as aggressive line breaks are placed when the lambda is not the last arg.
-  if ((Style.Language == FormatStyle::LK_Cpp ||
-   Style.Language == FormatStyle::LK_ObjC) &&
-  Left.is(tok::l_paren) && Left.BlockParameterCount > 0 &&
-  !Right.isOneOf(tok::l_paren, TT_LambdaLSquare)) {
-// Multiple lambdas in the same function call force line breaks.
-if (Left.BlockParameterCount > 1)
-  return true;
-
-// A lambda followed by another arg forces a line break.
-if (!Left.Role)
-  return false;
-auto Comma = Left.Role->lastComma();
-if (!Comma)
-  return false;
-auto Next = Comma->getNextNonComment();
-if (!Next)
-  return false;
-if (!Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret))
-  return true;
-  }
-
   return false;
 }
 
Index: clang/lib/Format/ContinuationIndenter.h
===
--- clang/lib/Format/ContinuationIndenter.h
+++ clang/lib/Format/ContinuationIndenter.h
@@ -433,6 +433,9 @@
   /// literal sequence, 0 otherwise.
   unsigned StartOfStringLiteral;
 
+  /// Disallow line breaks for this line.
+  bool NoLineBreak;
+
   /// A stack keeping track of properties applying to parenthesis
   /// levels.
   SmallVector Stack;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -260,6 +260,7 @@
/*NoLineBreak=*/false));
   State.NoContinuation = false;
   State.StartOfStringLiteral = 0;
+  State.NoLineBreak = false;
   State.StartOfLineLevel = 0;
   State.LowestLevelOnLine = 0;
   State.IgnoreStackForComparison = false;
@@ -342,7 +343,7 @@
 return true;
   }
 
-  return !CurrentState.NoLineBreak;
+  return !State.NoLineBreak && !CurrentState.NoLineBreak;
 }
 
 bool ContinuationIndenter::mustBreak(const LineState ) {
@@ -653,6 +654,38 @@
   const FormatToken  = *State.NextToken->Previous;
   auto  = State.Stack.back();
 
+  bool DisallowLineBreaksOnThisLine = Style.isCpp() && [] {
+// Deal with lambda arguments in C++. The aim here is to ensure that we
+// don't over-indent lambda function bodies when lambdas are passed as
+// arguments to 

[PATCH] D156259: [clang-format] Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 marked 4 inline comments as done.
jp4a50 added a comment.

Addressed all comments. Please let me know if there's anything else required 
before merging.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156259/new/

https://reviews.llvm.org/D156259

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

[PATCH] D156259: [clang-format] Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 552403.
jp4a50 added a comment.

Minor refactor.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156259/new/

https://reviews.llvm.org/D156259

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22177,8 +22177,25 @@
"  }\n"
"};");
 
-  // Multiple lambdas in the same parentheses change indentation rules. These
-  // lambdas are forced to start on new lines.
+  // Lambdas that fit on a single line within an argument list are not forced
+  // onto new lines.
+  verifyFormat("SomeFunction([] {});");
+  verifyFormat("SomeFunction(0, [] {});");
+  verifyFormat("SomeFunction([] {}, 0);");
+  verifyFormat("SomeFunction(0, [] {}, 0);");
+  verifyFormat("SomeFunction([] { return 0; }, 0);");
+  verifyFormat("SomeFunction(a, [] { return 0; }, b);");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; });");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  verifyFormat("auto lng =\n"
+   "SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  // Exceeded column limit. We need to break.
+  verifyFormat("auto lngName = SomeFunction(\n"
+   "[] { return anotherLooonngName; }, [] { "
+   "return 0; }, b);");
+
+  // Multiple multi-line lambdas in the same parentheses change indentation
+  // rules. These lambdas are always forced to start on new lines.
   verifyFormat("SomeFunction(\n"
"[]() {\n"
"  //\n"
@@ -22187,7 +22204,7 @@
"  //\n"
"});");
 
-  // A lambda passed as arg0 is always pushed to the next line.
+  // A multi-line lambda passed as arg0 is always pushed to the next line.
   verifyFormat("SomeFunction(\n"
"[this] {\n"
"  //\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -5203,30 +5203,6 @@
   return true;
   }
 
-  // Deal with lambda arguments in C++ - we want consistent line breaks whether
-  // they happen to be at arg0, arg1 or argN. The selection is a bit nuanced
-  // as aggressive line breaks are placed when the lambda is not the last arg.
-  if ((Style.Language == FormatStyle::LK_Cpp ||
-   Style.Language == FormatStyle::LK_ObjC) &&
-  Left.is(tok::l_paren) && Left.BlockParameterCount > 0 &&
-  !Right.isOneOf(tok::l_paren, TT_LambdaLSquare)) {
-// Multiple lambdas in the same function call force line breaks.
-if (Left.BlockParameterCount > 1)
-  return true;
-
-// A lambda followed by another arg forces a line break.
-if (!Left.Role)
-  return false;
-auto Comma = Left.Role->lastComma();
-if (!Comma)
-  return false;
-auto Next = Comma->getNextNonComment();
-if (!Next)
-  return false;
-if (!Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret))
-  return true;
-  }
-
   return false;
 }
 
Index: clang/lib/Format/ContinuationIndenter.h
===
--- clang/lib/Format/ContinuationIndenter.h
+++ clang/lib/Format/ContinuationIndenter.h
@@ -433,6 +433,9 @@
   /// literal sequence, 0 otherwise.
   unsigned StartOfStringLiteral;
 
+  /// Disallow line breaks for this line.
+  bool NoLineBreak;
+
   /// A stack keeping track of properties applying to parenthesis
   /// levels.
   SmallVector Stack;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -260,6 +260,7 @@
/*NoLineBreak=*/false));
   State.NoContinuation = false;
   State.StartOfStringLiteral = 0;
+  State.NoLineBreak = false;
   State.StartOfLineLevel = 0;
   State.LowestLevelOnLine = 0;
   State.IgnoreStackForComparison = false;
@@ -342,7 +343,7 @@
 return true;
   }
 
-  return !CurrentState.NoLineBreak;
+  return !State.NoLineBreak && !CurrentState.NoLineBreak;
 }
 
 bool ContinuationIndenter::mustBreak(const LineState ) {
@@ -653,6 +654,42 @@
   const FormatToken  = *State.NextToken->Previous;
   auto  = State.Stack.back();
 
+  bool DisallowLineBreaksOnThisLine =
+  (Style.Language == FormatStyle::LK_Cpp ||
+   Style.Language == FormatStyle::LK_ObjC) &&
+  [] {
+// Deal with lambda arguments in C++. The aim here is to ensure that we
+// 

[PATCH] D156259: [clang-format] Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 552401.
jp4a50 added a comment.

Minor refactoring and comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156259/new/

https://reviews.llvm.org/D156259

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22177,8 +22177,25 @@
"  }\n"
"};");
 
-  // Multiple lambdas in the same parentheses change indentation rules. These
-  // lambdas are forced to start on new lines.
+  // Lambdas that fit on a single line within an argument list are not forced
+  // onto new lines.
+  verifyFormat("SomeFunction([] {});");
+  verifyFormat("SomeFunction(0, [] {});");
+  verifyFormat("SomeFunction([] {}, 0);");
+  verifyFormat("SomeFunction(0, [] {}, 0);");
+  verifyFormat("SomeFunction([] { return 0; }, 0);");
+  verifyFormat("SomeFunction(a, [] { return 0; }, b);");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; });");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  verifyFormat("auto lng =\n"
+   "SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  // Exceeded column limit. We need to break.
+  verifyFormat("auto lngName = SomeFunction(\n"
+   "[] { return anotherLooonngName; }, [] { "
+   "return 0; }, b);");
+
+  // Multiple multi-line lambdas in the same parentheses change indentation
+  // rules. These lambdas are always forced to start on new lines.
   verifyFormat("SomeFunction(\n"
"[]() {\n"
"  //\n"
@@ -22187,7 +22204,7 @@
"  //\n"
"});");
 
-  // A lambda passed as arg0 is always pushed to the next line.
+  // A multi-line lambda passed as arg0 is always pushed to the next line.
   verifyFormat("SomeFunction(\n"
"[this] {\n"
"  //\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -5203,30 +5203,6 @@
   return true;
   }
 
-  // Deal with lambda arguments in C++ - we want consistent line breaks whether
-  // they happen to be at arg0, arg1 or argN. The selection is a bit nuanced
-  // as aggressive line breaks are placed when the lambda is not the last arg.
-  if ((Style.Language == FormatStyle::LK_Cpp ||
-   Style.Language == FormatStyle::LK_ObjC) &&
-  Left.is(tok::l_paren) && Left.BlockParameterCount > 0 &&
-  !Right.isOneOf(tok::l_paren, TT_LambdaLSquare)) {
-// Multiple lambdas in the same function call force line breaks.
-if (Left.BlockParameterCount > 1)
-  return true;
-
-// A lambda followed by another arg forces a line break.
-if (!Left.Role)
-  return false;
-auto Comma = Left.Role->lastComma();
-if (!Comma)
-  return false;
-auto Next = Comma->getNextNonComment();
-if (!Next)
-  return false;
-if (!Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret))
-  return true;
-  }
-
   return false;
 }
 
Index: clang/lib/Format/ContinuationIndenter.h
===
--- clang/lib/Format/ContinuationIndenter.h
+++ clang/lib/Format/ContinuationIndenter.h
@@ -433,6 +433,9 @@
   /// literal sequence, 0 otherwise.
   unsigned StartOfStringLiteral;
 
+  /// Disallow line breaks for this line.
+  bool NoLineBreak;
+
   /// A stack keeping track of properties applying to parenthesis
   /// levels.
   SmallVector Stack;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -260,6 +260,7 @@
/*NoLineBreak=*/false));
   State.NoContinuation = false;
   State.StartOfStringLiteral = 0;
+  State.NoLineBreak = false;
   State.StartOfLineLevel = 0;
   State.LowestLevelOnLine = 0;
   State.IgnoreStackForComparison = false;
@@ -342,7 +343,7 @@
 return true;
   }
 
-  return !CurrentState.NoLineBreak;
+  return !State.NoLineBreak && !CurrentState.NoLineBreak;
 }
 
 bool ContinuationIndenter::mustBreak(const LineState ) {
@@ -653,6 +654,44 @@
   const FormatToken  = *State.NextToken->Previous;
   auto  = State.Stack.back();
 
+  bool DisallowLineBreaksOnThisLine =
+  (Style.Language == FormatStyle::LK_Cpp ||
+   Style.Language == FormatStyle::LK_ObjC) &&
+  [] {
+// Deal with lambda arguments in C++. The aim here is to ensure 

[PATCH] D156259: [clang-format] Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

@HazardyKnusperkeks could you merge this for me assuming the build is green 
please? I don't have merge rights. Thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156259/new/

https://reviews.llvm.org/D156259

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

[PATCH] D156259: [clang-format] Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 546972.
jp4a50 added a comment.

Format files.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156259/new/

https://reviews.llvm.org/D156259

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22177,8 +22177,25 @@
"  }\n"
"};");
 
-  // Multiple lambdas in the same parentheses change indentation rules. These
-  // lambdas are forced to start on new lines.
+  // Lambdas that fit on a single line within an argument list are not forced
+  // onto new lines.
+  verifyFormat("SomeFunction([] {});");
+  verifyFormat("SomeFunction(0, [] {});");
+  verifyFormat("SomeFunction([] {}, 0);");
+  verifyFormat("SomeFunction(0, [] {}, 0);");
+  verifyFormat("SomeFunction([] { return 0; }, 0);");
+  verifyFormat("SomeFunction(a, [] { return 0; }, b);");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; });");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  verifyFormat("auto lng =\n"
+   "SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  // Exceeded column limit. We need to break.
+  verifyFormat("auto lngName = SomeFunction(\n"
+   "[] { return anotherLooonngName; }, [] { "
+   "return 0; }, b);");
+
+  // Multiple multi-line lambdas in the same parentheses change indentation
+  // rules. These lambdas are always forced to start on new lines.
   verifyFormat("SomeFunction(\n"
"[]() {\n"
"  //\n"
@@ -22187,7 +22204,7 @@
"  //\n"
"});");
 
-  // A lambda passed as arg0 is always pushed to the next line.
+  // A multi-line lambda passed as arg0 is always pushed to the next line.
   verifyFormat("SomeFunction(\n"
"[this] {\n"
"  //\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -5203,30 +5203,6 @@
   return true;
   }
 
-  // Deal with lambda arguments in C++ - we want consistent line breaks whether
-  // they happen to be at arg0, arg1 or argN. The selection is a bit nuanced
-  // as aggressive line breaks are placed when the lambda is not the last arg.
-  if ((Style.Language == FormatStyle::LK_Cpp ||
-   Style.Language == FormatStyle::LK_ObjC) &&
-  Left.is(tok::l_paren) && Left.BlockParameterCount > 0 &&
-  !Right.isOneOf(tok::l_paren, TT_LambdaLSquare)) {
-// Multiple lambdas in the same function call force line breaks.
-if (Left.BlockParameterCount > 1)
-  return true;
-
-// A lambda followed by another arg forces a line break.
-if (!Left.Role)
-  return false;
-auto Comma = Left.Role->lastComma();
-if (!Comma)
-  return false;
-auto Next = Comma->getNextNonComment();
-if (!Next)
-  return false;
-if (!Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret))
-  return true;
-  }
-
   return false;
 }
 
Index: clang/lib/Format/ContinuationIndenter.h
===
--- clang/lib/Format/ContinuationIndenter.h
+++ clang/lib/Format/ContinuationIndenter.h
@@ -433,6 +433,9 @@
   /// literal sequence, 0 otherwise.
   unsigned StartOfStringLiteral;
 
+  /// Disallow line breaks for this line.
+  bool NoLineBreak;
+
   /// A stack keeping track of properties applying to parenthesis
   /// levels.
   SmallVector Stack;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -260,6 +260,7 @@
/*NoLineBreak=*/false));
   State.NoContinuation = false;
   State.StartOfStringLiteral = 0;
+  State.NoLineBreak = false;
   State.StartOfLineLevel = 0;
   State.LowestLevelOnLine = 0;
   State.IgnoreStackForComparison = false;
@@ -342,7 +343,7 @@
 return true;
   }
 
-  return !CurrentState.NoLineBreak;
+  return !State.NoLineBreak && !CurrentState.NoLineBreak;
 }
 
 bool ContinuationIndenter::mustBreak(const LineState ) {
@@ -653,6 +654,47 @@
   const FormatToken  = *State.NextToken->Previous;
   auto  = State.Stack.back();
 
+  bool DisallowLineBreaksOnThisLine =
+  (Style.Language == FormatStyle::LK_Cpp ||
+   Style.Language == FormatStyle::LK_ObjC) &&
+  [] {
+// Deal with lambda arguments in C++. The aim here is 

[PATCH] D148131: Avoid unnecessarily aggressive line-breaking when using "LambdaBodyIndentation: OuterScope" with argument bin-packing.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 546004.
jp4a50 added a comment.

Stack diff on top of https://reviews.llvm.org/D156259.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148131/new/

https://reviews.llvm.org/D148131

Files:
  clang/docs/ReleaseNotes.rst
  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
@@ -22679,8 +22679,7 @@
"  []() -> auto {\n"
"int b = 32;\n"
"return 3;\n"
-   "  },\n"
-   "  foo, bar)\n"
+   "  }, foo, bar)\n"
"  .foo();\n"
"}",
Style);
@@ -22706,32 +22705,12 @@
"  })));\n"
"}",
Style);
-  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
-  verifyFormat("void foo() {\n"
-   "  aFunction(\n"
-   "  1, b(c(\n"
-   " [](d) -> Foo {\n"
-   "auto f = e(d);\n"
-   "return f;\n"
-   "  },\n"
-   " foo, Bar{},\n"
-   " [] {\n"
-   "auto g = h();\n"
-   "return g;\n"
-   "  },\n"
-   " baz)));\n"
-   "}",
-   Style);
   verifyFormat("void foo() {\n"
"  aFunction(1, b(c(foo, Bar{}, baz, [](d) -> Foo {\n"
-   "auto f = e(\n"
-   "foo,\n"
-   "[&] {\n"
+   "auto f = e(foo, [&] {\n"
"  auto g = h();\n"
"  return g;\n"
-   "},\n"
-   "qux,\n"
-   "[&] -> Bar {\n"
+   "}, qux, [&] -> Bar {\n"
"  auto i = j();\n"
"  return i;\n"
"});\n"
@@ -22739,28 +22718,74 @@
"  })));\n"
"}",
Style);
-  verifyFormat("Namespace::Foo::Foo(\n"
-   "LongClassName bar, AnotherLongClassName baz)\n"
+  verifyFormat("Namespace::Foo::Foo(LongClassName bar,\n"
+   "AnotherLongClassName baz)\n"
": baz{baz}, func{[&] {\n"
"  auto qux = bar;\n"
"  return aFunkyFunctionCall(qux);\n"
"}} {}",
Style);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  // As long as all the non-lambda arguments fit on a single line, AlwaysBreak
+  // doesn't force an initial line break, even if lambdas span multiple lines.
+  // This should probably be considered a bug.
+  verifyFormat("void foo() {\n"
+   "  aFunction([](d) -> Foo {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  }, foo, Bar{}, [] {\n"
+   "auto g = h();\n"
+   "return g;\n"
+   "  }, baz);\n"
+   "}",
+   Style);
+  // A long non-lambda argument forces arguments to span multiple lines and thus
+  // forces an initial line break when using AlwaysBreak.
+  verifyFormat("void foo() {\n"
+   "  aFunction(\n"
+   "  1,\n"
+   "  [](d) -> Foo {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  }, foo, Bar{},\n"
+   "  [] {\n"
+   "auto g = h();\n"
+   "return g;\n"
+   "  }, baz,\n"
+   "  qx);\n"
+   "}",
+   Style);
+  Style.BinPackArguments = false;
+  verifyFormat("void foo() {\n"
+   "  aFunction(\n"
+   "  1,\n"
+   "  [](d) -> Foo {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  },\n"
+   "  foo,\n"
+   "  Bar{},\n"
+   "  [] {\n"
+   "auto g = h();\n"
+   "return g;\n"
+   "  },\n"
+   "  baz,\n"
+   "  qx);\n"
+   "}",
+   Style);
+  Style.BinPackArguments = true;
   Style.BreakBeforeBraces = FormatStyle::BS_Custom;
   Style.BraceWrapping.BeforeLambdaBody = true;
   verifyFormat("void foo() {\n"
"  aFunction(\n"
-   "  1, b(c(foo, Bar{}, baz,\n"
-   " [](d) -> Foo\n"
+   "  1, b(c(foo, Bar{}, baz, [](d) -> Foo\n"
"  {\n"
"auto f = e(\n"
"

[PATCH] D156259: [clang-format] Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 546002.
jp4a50 added a comment.

More CR feedback.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156259/new/

https://reviews.llvm.org/D156259

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22177,8 +22177,25 @@
"  }\n"
"};");
 
-  // Multiple lambdas in the same parentheses change indentation rules. These
-  // lambdas are forced to start on new lines.
+  // Lambdas that fit on a single line within an argument list are not forced
+  // onto new lines.
+  verifyFormat("SomeFunction([] {});");
+  verifyFormat("SomeFunction(0, [] {});");
+  verifyFormat("SomeFunction([] {}, 0);");
+  verifyFormat("SomeFunction(0, [] {}, 0);");
+  verifyFormat("SomeFunction([] { return 0; }, 0);");
+  verifyFormat("SomeFunction(a, [] { return 0; }, b);");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; });");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  verifyFormat("auto lng =\n"
+   "SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  // Exceeded column limit. We need to break.
+  verifyFormat("auto lngName = SomeFunction(\n"
+   "[] { return anotherLooonngName; }, [] { "
+   "return 0; }, b);");
+
+  // Multiple multi-line lambdas in the same parentheses change indentation
+  // rules. These lambdas are always forced to start on new lines.
   verifyFormat("SomeFunction(\n"
"[]() {\n"
"  //\n"
@@ -22187,7 +22204,7 @@
"  //\n"
"});");
 
-  // A lambda passed as arg0 is always pushed to the next line.
+  // A multi-line lambda passed as arg0 is always pushed to the next line.
   verifyFormat("SomeFunction(\n"
"[this] {\n"
"  //\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -5203,30 +5203,6 @@
   return true;
   }
 
-  // Deal with lambda arguments in C++ - we want consistent line breaks whether
-  // they happen to be at arg0, arg1 or argN. The selection is a bit nuanced
-  // as aggressive line breaks are placed when the lambda is not the last arg.
-  if ((Style.Language == FormatStyle::LK_Cpp ||
-   Style.Language == FormatStyle::LK_ObjC) &&
-  Left.is(tok::l_paren) && Left.BlockParameterCount > 0 &&
-  !Right.isOneOf(tok::l_paren, TT_LambdaLSquare)) {
-// Multiple lambdas in the same function call force line breaks.
-if (Left.BlockParameterCount > 1)
-  return true;
-
-// A lambda followed by another arg forces a line break.
-if (!Left.Role)
-  return false;
-auto Comma = Left.Role->lastComma();
-if (!Comma)
-  return false;
-auto Next = Comma->getNextNonComment();
-if (!Next)
-  return false;
-if (!Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret))
-  return true;
-  }
-
   return false;
 }
 
Index: clang/lib/Format/ContinuationIndenter.h
===
--- clang/lib/Format/ContinuationIndenter.h
+++ clang/lib/Format/ContinuationIndenter.h
@@ -433,6 +433,9 @@
   /// literal sequence, 0 otherwise.
   unsigned StartOfStringLiteral;
 
+  /// Disallow line breaks for this line.
+  bool NoLineBreak;
+
   /// A stack keeping track of properties applying to parenthesis
   /// levels.
   SmallVector Stack;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -260,6 +260,7 @@
/*NoLineBreak=*/false));
   State.NoContinuation = false;
   State.StartOfStringLiteral = 0;
+  State.NoLineBreak = false;
   State.StartOfLineLevel = 0;
   State.LowestLevelOnLine = 0;
   State.IgnoreStackForComparison = false;
@@ -342,7 +343,7 @@
 return true;
   }
 
-  return !CurrentState.NoLineBreak;
+  return !State.NoLineBreak && !CurrentState.NoLineBreak;
 }
 
 bool ContinuationIndenter::mustBreak(const LineState ) {
@@ -653,6 +654,45 @@
   const FormatToken  = *State.NextToken->Previous;
   auto  = State.Stack.back();
 
+  bool DisallowLineBreaksOnThisLine = (Style.Language == FormatStyle::LK_Cpp ||
+Style.Language == FormatStyle::LK_ObjC) && [] {
+// Deal with lambda arguments in C++. The aim here is to ensure that 

[PATCH] D156259: [clang-format] Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:657-666
+  bool DisallowLineBreaksOnThisLine = [ = this->Style, ] {
+// Deal with lambda arguments in C++. The aim here is to ensure that we
+// don't over-indent lambda function bodies when lamdbas are passed as
+// arguments to function calls. We do this by ensuring that either all
+// arguments (including any lambdas) go on the same line as the function
+// call, or we break before the first argument.
+if (Style.Language != FormatStyle::LK_Cpp &&

HazardyKnusperkeks wrote:
> I'd go for this.
> But that's not necessary.
Ah, yes, I see what you mean, now! Easy enough change to make so I'll do it 
since there's a typo to fix as well anyway.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156259/new/

https://reviews.llvm.org/D156259

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

[PATCH] D156259: Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 545622.
jp4a50 marked an inline comment as done.
jp4a50 added a comment.

Formatting.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156259/new/

https://reviews.llvm.org/D156259

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22177,8 +22177,25 @@
"  }\n"
"};");
 
-  // Multiple lambdas in the same parentheses change indentation rules. These
-  // lambdas are forced to start on new lines.
+  // Lambdas that fit on a single line within an argument list are not forced
+  // onto new lines.
+  verifyFormat("SomeFunction([] {});");
+  verifyFormat("SomeFunction(0, [] {});");
+  verifyFormat("SomeFunction([] {}, 0);");
+  verifyFormat("SomeFunction(0, [] {}, 0);");
+  verifyFormat("SomeFunction([] { return 0; }, 0);");
+  verifyFormat("SomeFunction(a, [] { return 0; }, b);");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; });");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  verifyFormat("auto lng =\n"
+   "SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  // Exceeded column limit. We need to break.
+  verifyFormat("auto lngName = SomeFunction(\n"
+   "[] { return anotherLooonngName; }, [] { "
+   "return 0; }, b);");
+
+  // Multiple multi-line lambdas in the same parentheses change indentation
+  // rules. These lambdas are always forced to start on new lines.
   verifyFormat("SomeFunction(\n"
"[]() {\n"
"  //\n"
@@ -22187,7 +22204,7 @@
"  //\n"
"});");
 
-  // A lambda passed as arg0 is always pushed to the next line.
+  // A multi-line lambda passed as arg0 is always pushed to the next line.
   verifyFormat("SomeFunction(\n"
"[this] {\n"
"  //\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -5203,30 +5203,6 @@
   return true;
   }
 
-  // Deal with lambda arguments in C++ - we want consistent line breaks whether
-  // they happen to be at arg0, arg1 or argN. The selection is a bit nuanced
-  // as aggressive line breaks are placed when the lambda is not the last arg.
-  if ((Style.Language == FormatStyle::LK_Cpp ||
-   Style.Language == FormatStyle::LK_ObjC) &&
-  Left.is(tok::l_paren) && Left.BlockParameterCount > 0 &&
-  !Right.isOneOf(tok::l_paren, TT_LambdaLSquare)) {
-// Multiple lambdas in the same function call force line breaks.
-if (Left.BlockParameterCount > 1)
-  return true;
-
-// A lambda followed by another arg forces a line break.
-if (!Left.Role)
-  return false;
-auto Comma = Left.Role->lastComma();
-if (!Comma)
-  return false;
-auto Next = Comma->getNextNonComment();
-if (!Next)
-  return false;
-if (!Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret))
-  return true;
-  }
-
   return false;
 }
 
Index: clang/lib/Format/ContinuationIndenter.h
===
--- clang/lib/Format/ContinuationIndenter.h
+++ clang/lib/Format/ContinuationIndenter.h
@@ -433,6 +433,9 @@
   /// literal sequence, 0 otherwise.
   unsigned StartOfStringLiteral;
 
+  /// Disallow line breaks for this line.
+  bool NoLineBreak;
+
   /// A stack keeping track of properties applying to parenthesis
   /// levels.
   SmallVector Stack;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -260,6 +260,7 @@
/*NoLineBreak=*/false));
   State.NoContinuation = false;
   State.StartOfStringLiteral = 0;
+  State.NoLineBreak = false;
   State.StartOfLineLevel = 0;
   State.LowestLevelOnLine = 0;
   State.IgnoreStackForComparison = false;
@@ -342,7 +343,7 @@
 return true;
   }
 
-  return !CurrentState.NoLineBreak;
+  return !State.NoLineBreak && !CurrentState.NoLineBreak;
 }
 
 bool ContinuationIndenter::mustBreak(const LineState ) {
@@ -653,6 +654,48 @@
   const FormatToken  = *State.NextToken->Previous;
   auto  = State.Stack.back();
 
+  bool DisallowLineBreaksOnThisLine = [ = this->Style, ] {
+// Deal with lambda arguments in C++. The aim here is to ensure that we
+// don't over-indent lambda 

[PATCH] D156259: Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 marked 3 inline comments as done.
jp4a50 added a comment.

All comments addressed. PTAL and let me know if there are any other blockers 
for merge.




Comment at: clang/lib/Format/ContinuationIndenter.cpp:664-666
+if ((Style.Language == FormatStyle::LK_Cpp ||
+ Style.Language == FormatStyle::LK_ObjC) &&
+!Current.is(tok::comment) && PrevNonComment &&

HazardyKnusperkeks wrote:
> jp4a50 wrote:
> > HazardyKnusperkeks wrote:
> > > jp4a50 wrote:
> > > > HazardyKnusperkeks wrote:
> > > > > Could move this out of the lambda.
> > > > Not sure exactly what you're referring to here. Initialization of 
> > > > `PrevNonComment`?
> > > > Not sure exactly what you're referring to here. Initialization of 
> > > > `PrevNonComment`?
> > > 
> > > No everything in the if before you go into the `PrevNonComment`. It is 
> > > highlighted if you hover the comment.
> > Ah, I see, thanks. Technically I could move that outside the lambda. I'm 
> > just not sure how it's better? Are you making the suggestion because it 
> > would make it clearer that this logic only applies to cpp/objc and/or 
> > because it means we could avoid capturing `Style`?
> Can do how you want, I think avoid capturing `Style` would be nice. And you 
> wouldn't search for `PrevNonComment` if you don't need to. This could also be 
> done be reordering the statements and a new `return`.
I kept all conditions inside the lambda in the end since moving any out would 
have made the `DisallowLineBreaksOnThisLine` not reflect all conditions and I 
wanted to keep that.

Instead I've changed all conditions to the style of early return for 
consistency and ordered them in the way that I think makes them most readable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156259/new/

https://reviews.llvm.org/D156259

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

[PATCH] D156259: Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 545594.
jp4a50 added a comment.

Refactor DisallowLineBreaksOnThisLine lambda.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156259/new/

https://reviews.llvm.org/D156259

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22177,8 +22177,25 @@
"  }\n"
"};");
 
-  // Multiple lambdas in the same parentheses change indentation rules. These
-  // lambdas are forced to start on new lines.
+  // Lambdas that fit on a single line within an argument list are not forced
+  // onto new lines.
+  verifyFormat("SomeFunction([] {});");
+  verifyFormat("SomeFunction(0, [] {});");
+  verifyFormat("SomeFunction([] {}, 0);");
+  verifyFormat("SomeFunction(0, [] {}, 0);");
+  verifyFormat("SomeFunction([] { return 0; }, 0);");
+  verifyFormat("SomeFunction(a, [] { return 0; }, b);");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; });");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  verifyFormat("auto lng =\n"
+   "SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  // Exceeded column limit. We need to break.
+  verifyFormat("auto lngName = SomeFunction(\n"
+   "[] { return anotherLooonngName; }, [] { "
+   "return 0; }, b);");
+
+  // Multiple multi-line lambdas in the same parentheses change indentation
+  // rules. These lambdas are always forced to start on new lines.
   verifyFormat("SomeFunction(\n"
"[]() {\n"
"  //\n"
@@ -22187,7 +22204,7 @@
"  //\n"
"});");
 
-  // A lambda passed as arg0 is always pushed to the next line.
+  // A multi-line lambda passed as arg0 is always pushed to the next line.
   verifyFormat("SomeFunction(\n"
"[this] {\n"
"  //\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -5203,30 +5203,6 @@
   return true;
   }
 
-  // Deal with lambda arguments in C++ - we want consistent line breaks whether
-  // they happen to be at arg0, arg1 or argN. The selection is a bit nuanced
-  // as aggressive line breaks are placed when the lambda is not the last arg.
-  if ((Style.Language == FormatStyle::LK_Cpp ||
-   Style.Language == FormatStyle::LK_ObjC) &&
-  Left.is(tok::l_paren) && Left.BlockParameterCount > 0 &&
-  !Right.isOneOf(tok::l_paren, TT_LambdaLSquare)) {
-// Multiple lambdas in the same function call force line breaks.
-if (Left.BlockParameterCount > 1)
-  return true;
-
-// A lambda followed by another arg forces a line break.
-if (!Left.Role)
-  return false;
-auto Comma = Left.Role->lastComma();
-if (!Comma)
-  return false;
-auto Next = Comma->getNextNonComment();
-if (!Next)
-  return false;
-if (!Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret))
-  return true;
-  }
-
   return false;
 }
 
Index: clang/lib/Format/ContinuationIndenter.h
===
--- clang/lib/Format/ContinuationIndenter.h
+++ clang/lib/Format/ContinuationIndenter.h
@@ -433,6 +433,9 @@
   /// literal sequence, 0 otherwise.
   unsigned StartOfStringLiteral;
 
+  /// Disallow line breaks for this line.
+  bool NoLineBreak;
+
   /// A stack keeping track of properties applying to parenthesis
   /// levels.
   SmallVector Stack;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -260,6 +260,7 @@
/*NoLineBreak=*/false));
   State.NoContinuation = false;
   State.StartOfStringLiteral = 0;
+  State.NoLineBreak = false;
   State.StartOfLineLevel = 0;
   State.LowestLevelOnLine = 0;
   State.IgnoreStackForComparison = false;
@@ -342,7 +343,7 @@
 return true;
   }
 
-  return !CurrentState.NoLineBreak;
+  return !State.NoLineBreak && !CurrentState.NoLineBreak;
 }
 
 bool ContinuationIndenter::mustBreak(const LineState ) {
@@ -653,6 +654,47 @@
   const FormatToken  = *State.NextToken->Previous;
   auto  = State.Stack.back();
 
+  bool DisallowLineBreaksOnThisLine = [ = this->Style, ] {
+// Deal with lambda arguments in C++. The aim here is to ensure that we
+// don't over-indent lambda function 

[PATCH] D156259: Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:664-666
+if ((Style.Language == FormatStyle::LK_Cpp ||
+ Style.Language == FormatStyle::LK_ObjC) &&
+!Current.is(tok::comment) && PrevNonComment &&

HazardyKnusperkeks wrote:
> jp4a50 wrote:
> > HazardyKnusperkeks wrote:
> > > Could move this out of the lambda.
> > Not sure exactly what you're referring to here. Initialization of 
> > `PrevNonComment`?
> > Not sure exactly what you're referring to here. Initialization of 
> > `PrevNonComment`?
> 
> No everything in the if before you go into the `PrevNonComment`. It is 
> highlighted if you hover the comment.
Ah, I see, thanks. Technically I could move that outside the lambda. I'm just 
not sure how it's better? Are you making the suggestion because it would make 
it clearer that this logic only applies to cpp/objc and/or because it means we 
could avoid capturing `Style`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156259/new/

https://reviews.llvm.org/D156259

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

[PATCH] D156259: Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 544310.
jp4a50 added a comment.

Format files.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156259/new/

https://reviews.llvm.org/D156259

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22177,8 +22177,25 @@
"  }\n"
"};");
 
-  // Multiple lambdas in the same parentheses change indentation rules. These
-  // lambdas are forced to start on new lines.
+  // Lambdas that fit on a single line within an argument list are not forced
+  // onto new lines.
+  verifyFormat("SomeFunction([] {});");
+  verifyFormat("SomeFunction(0, [] {});");
+  verifyFormat("SomeFunction([] {}, 0);");
+  verifyFormat("SomeFunction(0, [] {}, 0);");
+  verifyFormat("SomeFunction([] { return 0; }, 0);");
+  verifyFormat("SomeFunction(a, [] { return 0; }, b);");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; });");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  verifyFormat("auto lng =\n"
+   "SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  // Exceeded column limit. We need to break.
+  verifyFormat("auto lngName = SomeFunction(\n"
+   "[] { return anotherLooonngName; }, [] { "
+   "return 0; }, b);");
+
+  // Multiple multi-line lambdas in the same parentheses change indentation
+  // rules. These lambdas are always forced to start on new lines.
   verifyFormat("SomeFunction(\n"
"[]() {\n"
"  //\n"
@@ -22187,7 +22204,7 @@
"  //\n"
"});");
 
-  // A lambda passed as arg0 is always pushed to the next line.
+  // A multi-line lambda passed as arg0 is always pushed to the next line.
   verifyFormat("SomeFunction(\n"
"[this] {\n"
"  //\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -5203,30 +5203,6 @@
   return true;
   }
 
-  // Deal with lambda arguments in C++ - we want consistent line breaks whether
-  // they happen to be at arg0, arg1 or argN. The selection is a bit nuanced
-  // as aggressive line breaks are placed when the lambda is not the last arg.
-  if ((Style.Language == FormatStyle::LK_Cpp ||
-   Style.Language == FormatStyle::LK_ObjC) &&
-  Left.is(tok::l_paren) && Left.BlockParameterCount > 0 &&
-  !Right.isOneOf(tok::l_paren, TT_LambdaLSquare)) {
-// Multiple lambdas in the same function call force line breaks.
-if (Left.BlockParameterCount > 1)
-  return true;
-
-// A lambda followed by another arg forces a line break.
-if (!Left.Role)
-  return false;
-auto Comma = Left.Role->lastComma();
-if (!Comma)
-  return false;
-auto Next = Comma->getNextNonComment();
-if (!Next)
-  return false;
-if (!Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret))
-  return true;
-  }
-
   return false;
 }
 
Index: clang/lib/Format/ContinuationIndenter.h
===
--- clang/lib/Format/ContinuationIndenter.h
+++ clang/lib/Format/ContinuationIndenter.h
@@ -433,6 +433,9 @@
   /// literal sequence, 0 otherwise.
   unsigned StartOfStringLiteral;
 
+  /// Disallow line breaks for this line.
+  bool NoLineBreak;
+
   /// A stack keeping track of properties applying to parenthesis
   /// levels.
   SmallVector Stack;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -260,6 +260,7 @@
/*NoLineBreak=*/false));
   State.NoContinuation = false;
   State.StartOfStringLiteral = 0;
+  State.NoLineBreak = false;
   State.StartOfLineLevel = 0;
   State.LowestLevelOnLine = 0;
   State.IgnoreStackForComparison = false;
@@ -342,7 +343,7 @@
 return true;
   }
 
-  return !CurrentState.NoLineBreak;
+  return !State.NoLineBreak && !CurrentState.NoLineBreak;
 }
 
 bool ContinuationIndenter::mustBreak(const LineState ) {
@@ -653,6 +654,39 @@
   const FormatToken  = *State.NextToken->Previous;
   auto  = State.Stack.back();
 
+  bool DisallowLineBreaksOnThisLine = [ = this->Style, ] {
+// Deal with lambda arguments in C++. The aim here is to ensure that we
+// don't over-indent lambda function bodies when lamdbas are passed as
+  

[PATCH] D156259: Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 marked 2 inline comments as done.
jp4a50 added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:263
   State.StartOfStringLiteral = 0;
+  State.NoLineBreak = false;
   State.StartOfLineLevel = 0;

HazardyKnusperkeks wrote:
> Was this uninitialized?
It's a new member variable I added as part of this diff.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:664-666
+if ((Style.Language == FormatStyle::LK_Cpp ||
+ Style.Language == FormatStyle::LK_ObjC) &&
+!Current.is(tok::comment) && PrevNonComment &&

HazardyKnusperkeks wrote:
> Could move this out of the lambda.
Not sure exactly what you're referring to here. Initialization of 
`PrevNonComment`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156259/new/

https://reviews.llvm.org/D156259

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

[PATCH] D156259: Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 544308.
jp4a50 added a comment.

Address minor review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156259/new/

https://reviews.llvm.org/D156259

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22177,8 +22177,25 @@
"  }\n"
"};");
 
-  // Multiple lambdas in the same parentheses change indentation rules. These
-  // lambdas are forced to start on new lines.
+  // Lambdas that fit on a single line within an argument list are not forced
+  // onto new lines.
+  verifyFormat("SomeFunction([] {});");
+  verifyFormat("SomeFunction(0, [] {});");
+  verifyFormat("SomeFunction([] {}, 0);");
+  verifyFormat("SomeFunction(0, [] {}, 0);");
+  verifyFormat("SomeFunction([] { return 0; }, 0);");
+  verifyFormat("SomeFunction(a, [] { return 0; }, b);");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; });");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  verifyFormat("auto lng =\n"
+   "SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  // Exceeded column limit. We need to break.
+  verifyFormat("auto lngName = SomeFunction(\n"
+   "[] { return anotherLooonngName; }, [] { "
+   "return 0; }, b);");
+
+  // Multiple multi-line lambdas in the same parentheses change indentation
+  // rules. These lambdas are always forced to start on new lines.
   verifyFormat("SomeFunction(\n"
"[]() {\n"
"  //\n"
@@ -22187,7 +22204,7 @@
"  //\n"
"});");
 
-  // A lambda passed as arg0 is always pushed to the next line.
+  // A multi-line lambda passed as arg0 is always pushed to the next line.
   verifyFormat("SomeFunction(\n"
"[this] {\n"
"  //\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -5203,30 +5203,6 @@
   return true;
   }
 
-  // Deal with lambda arguments in C++ - we want consistent line breaks whether
-  // they happen to be at arg0, arg1 or argN. The selection is a bit nuanced
-  // as aggressive line breaks are placed when the lambda is not the last arg.
-  if ((Style.Language == FormatStyle::LK_Cpp ||
-   Style.Language == FormatStyle::LK_ObjC) &&
-  Left.is(tok::l_paren) && Left.BlockParameterCount > 0 &&
-  !Right.isOneOf(tok::l_paren, TT_LambdaLSquare)) {
-// Multiple lambdas in the same function call force line breaks.
-if (Left.BlockParameterCount > 1)
-  return true;
-
-// A lambda followed by another arg forces a line break.
-if (!Left.Role)
-  return false;
-auto Comma = Left.Role->lastComma();
-if (!Comma)
-  return false;
-auto Next = Comma->getNextNonComment();
-if (!Next)
-  return false;
-if (!Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret))
-  return true;
-  }
-
   return false;
 }
 
Index: clang/lib/Format/ContinuationIndenter.h
===
--- clang/lib/Format/ContinuationIndenter.h
+++ clang/lib/Format/ContinuationIndenter.h
@@ -433,6 +433,9 @@
   /// literal sequence, 0 otherwise.
   unsigned StartOfStringLiteral;
 
+  /// Disallow line breaks for this line.
+  bool NoLineBreak;
+
   /// A stack keeping track of properties applying to parenthesis
   /// levels.
   SmallVector Stack;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -260,6 +260,7 @@
/*NoLineBreak=*/false));
   State.NoContinuation = false;
   State.StartOfStringLiteral = 0;
+  State.NoLineBreak = false;
   State.StartOfLineLevel = 0;
   State.LowestLevelOnLine = 0;
   State.IgnoreStackForComparison = false;
@@ -342,7 +343,7 @@
 return true;
   }
 
-  return !CurrentState.NoLineBreak;
+  return !State.NoLineBreak && !CurrentState.NoLineBreak;
 }
 
 bool ContinuationIndenter::mustBreak(const LineState ) {
@@ -653,6 +654,40 @@
   const FormatToken  = *State.NextToken->Previous;
   auto  = State.Stack.back();
 
+  bool DisallowLineBreaksOnThisLine = [ = this->Style, ] {
+// Deal with lambda arguments in C++. The aim here is to ensure that we
+// don't over-indent lambda function bodies when lamdbas 

[PATCH] D148131: Avoid unnecessarily aggressive line-breaking when using "LambdaBodyIndentation: OuterScope" with argument bin-packing.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

@owenpan I've put up a patch for the bug you mentioned here: 
https://reviews.llvm.org/D156259

I'll rebase this change on top of that if you like the look of it. It should be 
fairly trivial to do.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148131/new/

https://reviews.llvm.org/D148131

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

[PATCH] D156259: Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 created this revision.
Herald added projects: All, clang, clang-format.
Herald added a subscriber: cfe-commits.
Herald added reviewers: rymiel, HazardyKnusperkeks, owenpan, MyDeveloperDay.
jp4a50 requested review of this revision.

Fixes a long-standing bug: https://github.com/llvm/llvm-project/issues/44486.

The original diff that introduced the bug implemented behaviour that pushed the 
first argument to a function onto a new line under certain circumstances 
relating passing lambdas as arguments.

This behaviour was implemented in TokenAnnotator::mustBreakBefore() which meant 
the code lacked the necessary context to figure out whether subsequent 
arguments might be able to all fit on one line. As such, I've moved the 
implementation to ContinuationIndenter and, instead of forcing a line break at 
the first argument in all cases, we now allow the OptimizingLineFormatter to 
consider placing the first argument on the same line as the function call but 
don't allow further line breaks in this case.

The end result is that either the first argument must go on a new line (as 
before) or all arguments must be put on the current line.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156259

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22177,8 +22177,25 @@
"  }\n"
"};");
 
-  // Multiple lambdas in the same parentheses change indentation rules. These
-  // lambdas are forced to start on new lines.
+  // Lambdas that fit on a single line within an argument list are not forced
+  // onto new lines.
+  verifyFormat("SomeFunction([] {});");
+  verifyFormat("SomeFunction(0, [] {});");
+  verifyFormat("SomeFunction([] {}, 0);");
+  verifyFormat("SomeFunction(0, [] {}, 0);");
+  verifyFormat("SomeFunction([] { return 0; }, 0);");
+  verifyFormat("SomeFunction(a, [] { return 0; }, b);");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; });");
+  verifyFormat("SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  verifyFormat("auto lng =\n"
+   "SomeFunction([] { return 0; }, [] { return 0; }, b);");
+  // Exceeded column limit. We need to break.
+  verifyFormat("auto lngName = SomeFunction(\n"
+   "[] { return anotherLooonngName; }, [] { "
+   "return 0; }, b);");
+
+  // Multiple multi-line lambdas in the same parentheses change indentation
+  // rules. These lambdas are always forced to start on new lines.
   verifyFormat("SomeFunction(\n"
"[]() {\n"
"  //\n"
@@ -22187,7 +22204,7 @@
"  //\n"
"});");
 
-  // A lambda passed as arg0 is always pushed to the next line.
+  // A multi-line lambda passed as arg0 is always pushed to the next line.
   verifyFormat("SomeFunction(\n"
"[this] {\n"
"  //\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -5203,30 +5203,6 @@
   return true;
   }
 
-  // Deal with lambda arguments in C++ - we want consistent line breaks whether
-  // they happen to be at arg0, arg1 or argN. The selection is a bit nuanced
-  // as aggressive line breaks are placed when the lambda is not the last arg.
-  if ((Style.Language == FormatStyle::LK_Cpp ||
-   Style.Language == FormatStyle::LK_ObjC) &&
-  Left.is(tok::l_paren) && Left.BlockParameterCount > 0 &&
-  !Right.isOneOf(tok::l_paren, TT_LambdaLSquare)) {
-// Multiple lambdas in the same function call force line breaks.
-if (Left.BlockParameterCount > 1)
-  return true;
-
-// A lambda followed by another arg forces a line break.
-if (!Left.Role)
-  return false;
-auto Comma = Left.Role->lastComma();
-if (!Comma)
-  return false;
-auto Next = Comma->getNextNonComment();
-if (!Next)
-  return false;
-if (!Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret))
-  return true;
-  }
-
   return false;
 }
 
Index: clang/lib/Format/ContinuationIndenter.h
===
--- clang/lib/Format/ContinuationIndenter.h
+++ clang/lib/Format/ContinuationIndenter.h
@@ -433,6 +433,9 @@
   /// literal sequence, 0 otherwise.
   unsigned StartOfStringLiteral;
 
+  /// Disallow line breaks for this line.
+  bool NoLineBreak;
+
   /// A stack keeping track of properties applying to parenthesis
   /// levels.
   SmallVector Stack;
Index: 

[PATCH] D148131: Avoid unnecessarily aggressive line-breaking when using "LambdaBodyIndentation: OuterScope" with argument bin-packing.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

In D148131#4447426 , @owenpan wrote:

> In D148131#4304960 , @jp4a50 wrote:
>
>> Hey @MyDeveloperDay @owenpan . I'd appreciate if you guys have time to 
>> consider this change. It's the last significant issue my team and I are 
>> tracking that's blocking our adoption of clang-format for multiple codebases 
>> that use KJ 
>>  :)
>
> We probably should fix https://github.com/llvm/llvm-project/issues/44486 
> first.

Hi @owenpan . Thanks for your reply. Sorry that I missed it for a while - I 
stopped monitoring the review after a while and missed the email notification 
because I hadn't figured out how to make my LLVM email notifications only send 
me notifications related to my reviews.

I've had a bit of a look into https://github.com/llvm/llvm-project/issues/44486 
and I can see why it's happening. It is caused by the unconditional, aggressive 
line-breaking introduced by https://reviews.llvm.org/D52676 as mkurdej notes on 
the github issue.

Unfortunately I think the fact that the changes in 
https://reviews.llvm.org/D52676 were implemented in the TokenAnnotator (rather 
than ContinuationIndenter) means this will be a non-trivial fix - I don't think 
the decision to force these line breaks should have been implemented here as 
there is not enough context to handle edge cases like the one raised in 
https://github.com/llvm/llvm-project/issues/44486.

I'll have a go at moving the implementation of this behaviour into 
ContinuationIndenter to see if it allows me to fix the issue. My rough idea is 
to get the OptimizingLineFormatter to consider either breaking or not breaking 
before the first argument, but when we don't break before the first argument, 
disallow line-breaking before any other arguments (in all cases that we have 
lambda arguments except when there is a single lambda that is the last 
argument).

Hopefully that should allow us to maintain the same logic as we currently have 
but also consider the additional case of having all arguments (including the 
lambdas) on the same line. I doubt it will be as simple as that but thought I'd 
give you a heads up.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148131/new/

https://reviews.llvm.org/D148131

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

[PATCH] D148131: Avoid unnecessarily aggressive line-breaking when using "LambdaBodyIndentation: OuterScope" with argument bin-packing.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

Hey @MyDeveloperDay @owenpan . I'd appreciate if you guys have time to consider 
this change. It's the last significant issue my team and I are tracking that's 
blocking our adoption of clang-format for multiple codebases that use KJ 
 :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148131/new/

https://reviews.llvm.org/D148131

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

[PATCH] D146101: [clang-format] Add BracedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

In D146101#4270509 , @owenpan wrote:

> In D146101#4268487 , @jp4a50 wrote:
>
>> Would appreciate someone committing for me.
>
> Can you rebase it? I got a conflict in ReleaseNotes.rst.

@owenpan apologies for the delay. I've rebased now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146101: [clang-format] Add BracedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 517868.
jp4a50 added a comment.

Rebase.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/docs/tools/dump_format_style.py
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -4855,6 +4855,191 @@
"[5] = ee};");
 }
 
+TEST_F(FormatTest, BracedInitializerIndentWidth) {
+  auto Style = getLLVMStyleWithColumns(60);
+  Style.BinPackArguments = true;
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BracedInitializerIndentWidth = 6;
+
+  // Non-initializing braces are unaffected by BracedInitializerIndentWidth.
+  verifyFormat("enum class {\n"
+   "  One,\n"
+   "  Two,\n"
+   "};\n",
+   Style);
+  verifyFormat("class Foo {\n"
+   "  Foo() {}\n"
+   "  void bar();\n"
+   "};\n",
+   Style);
+  verifyFormat("void foo() {\n"
+   "  auto bar = baz;\n"
+   "  return baz;\n"
+   "};\n",
+   Style);
+  verifyFormat("auto foo = [&] {\n"
+   "  auto bar = baz;\n"
+   "  return baz;\n"
+   "};\n",
+   Style);
+  verifyFormat("{\n"
+   "  auto bar = baz;\n"
+   "  return baz;\n"
+   "};\n",
+   Style);
+  // Non-brace initialization is unaffected by BracedInitializerIndentWidth.
+  verifyFormat("SomeClass clazz(\n"
+   "\"xx\", \"yy\",\n"
+   "\"zz\");\n",
+   Style);
+
+  // The following types of initialization are all affected by
+  // BracedInitializerIndentWidth. Aggregate initialization.
+  verifyFormat("int LongVariable[2] = {\n"
+   "  1000, 2000};",
+   Style);
+  verifyFormat("SomeStruct s{\n"
+   "  \"\", \"\",\n"
+   "  \"\"};\n",
+   Style);
+  // Designated initializers.
+  verifyFormat("int LongVariable[1] = {\n"
+   "  [0] = 1000, [1] = 2000};",
+   Style);
+  verifyFormat("SomeStruct s{\n"
+   "  .foo = \"x\",\n"
+   "  .bar = \"y\",\n"
+   "  .baz = \"z\"};\n",
+   Style);
+  // List initialization.
+  verifyFormat("SomeStruct s{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  verifyFormat("SomeStruct{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  verifyFormat("new SomeStruct{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  // Member initializer.
+  verifyFormat("class SomeClass {\n"
+   "  SomeStruct s{\n"
+   "\"x\",\n"
+   "\"y\",\n"
+   "\"z\",\n"
+   "  };\n"
+   "};\n",
+   Style);
+  // Constructor member initializer.
+  verifyFormat("SomeClass::SomeClass : strct{\n"
+   " \"x\",\n"
+   " \"y\",\n"
+   " \"z\",\n"
+   "   } {}\n",
+   Style);
+  // Copy initialization.
+  verifyFormat("SomeStruct s = SomeStruct{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  // Copy list initialization.
+  verifyFormat("SomeStruct s = {\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  // Assignment operand initialization.
+  verifyFormat("s = {\n"
+   "  \"x\",\n"
+   "  

[PATCH] D146101: [clang-format] Add BracedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

Commit details as follows as per other diffs:

Name: Jon Phillips
Email: jonap2...@gmail.com

Would appreciate someone committing for me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D148131: Avoid unnecessarily aggressive line-breaking when using "LambdaBodyIndentation: OuterScope" with argument bin-packing.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:333
+  // enabled.
+  !(Current.is(TT_LambdaLBrace) && Style.BraceWrapping.BeforeLambdaBody) &&
   CurrentState.NoLineBreakInOperand) {

AFAICT, this only matters when using `OuterScope`, but it seems like a sensible 
exception to make in the general case, so I haven't included a check for 
`OuterScope` here.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1040
+  //
+  // We specifically special case "OuterScope"-formatted lambdas here
+  // because, when using that setting, breaking before the parameter

So I genuinely think that the original implementation here (which I've left 
intact *except* for when OuterScope is enabled) does not match up to the 
intention expressed in the comment in line 1018.

It effectively seems to assume that the parenthesis level entry which is second 
from the top is the 'parent' parenthesis level, but this ignores the fact that 
there are often "fake parens" entries in the parenthesis state stack which 
presumably should be ignored (as I have done in my implementation for 
OuterScope).

As such, there are a lot of cases where (when OuterScope is not enabled) we 
apply `BreakBeforeParameter` to the *existing* parenthesis level rather than 
the parent parenthesis level. So when I tried to "fix" this behaviour in the 
general case it broke a lot of tests, even though I think the behaviour may be 
what was originally intended.

Happy to discuss this and be proven wrong, but I thought I'd explain why I've 
added a special case here for OuterScope in even greater detail than I have in 
the comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148131/new/

https://reviews.llvm.org/D148131

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

[PATCH] D148131: Avoid unnecessarily aggressive line-breaking when using "LambdaBodyIndentation: OuterScope" with argument bin-packing.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 created this revision.
Herald added projects: All, clang, clang-format.
Herald added a subscriber: cfe-commits.
Herald added reviewers: rymiel, HazardyKnusperkeks, owenpan, MyDeveloperDay.
jp4a50 requested review of this revision.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148131

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22011,8 +22011,7 @@
"  []() -> auto {\n"
"int b = 32;\n"
"return 3;\n"
-   "  },\n"
-   "  foo, bar)\n"
+   "  }, foo, bar)\n"
"  .foo();\n"
"}",
Style);
@@ -22038,32 +22037,12 @@
"  })));\n"
"}",
Style);
-  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
-  verifyFormat("void foo() {\n"
-   "  aFunction(\n"
-   "  1, b(c(\n"
-   " [](d) -> Foo {\n"
-   "auto f = e(d);\n"
-   "return f;\n"
-   "  },\n"
-   " foo, Bar{},\n"
-   " [] {\n"
-   "auto g = h();\n"
-   "return g;\n"
-   "  },\n"
-   " baz)));\n"
-   "}",
-   Style);
   verifyFormat("void foo() {\n"
"  aFunction(1, b(c(foo, Bar{}, baz, [](d) -> Foo {\n"
-   "auto f = e(\n"
-   "foo,\n"
-   "[&] {\n"
+   "auto f = e(foo, [&] {\n"
"  auto g = h();\n"
"  return g;\n"
-   "},\n"
-   "qux,\n"
-   "[&] -> Bar {\n"
+   "}, qux, [&] -> Bar {\n"
"  auto i = j();\n"
"  return i;\n"
"});\n"
@@ -22071,28 +22050,74 @@
"  })));\n"
"}",
Style);
-  verifyFormat("Namespace::Foo::Foo(\n"
-   "LongClassName bar, AnotherLongClassName baz)\n"
+  verifyFormat("Namespace::Foo::Foo(LongClassName bar,\n"
+   "AnotherLongClassName baz)\n"
": baz{baz}, func{[&] {\n"
"  auto qux = bar;\n"
"  return aFunkyFunctionCall(qux);\n"
"}} {}",
Style);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  // As long as all the non-lambda arguments fit on a single line, AlwaysBreak
+  // doesn't force an initial line break, even if lambdas span multiple lines.
+  // This should probably be considered a bug.
+  verifyFormat("void foo() {\n"
+   "  aFunction([](d) -> Foo {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  }, foo, Bar{}, [] {\n"
+   "auto g = h();\n"
+   "return g;\n"
+   "  }, baz);\n"
+   "}",
+   Style);
+  // A long non-lambda argument forces arguments to span multiple lines and thus
+  // forces an initial line break when using AlwaysBreak.
+  verifyFormat("void foo() {\n"
+   "  aFunction(\n"
+   "  1,\n"
+   "  [](d) -> Foo {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  }, foo, Bar{},\n"
+   "  [] {\n"
+   "auto g = h();\n"
+   "return g;\n"
+   "  }, baz,\n"
+   "  qx);\n"
+   "}",
+   Style);
+  Style.BinPackArguments = false;
+  verifyFormat("void foo() {\n"
+   "  aFunction(\n"
+   "  1,\n"
+   "  [](d) -> Foo {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  },\n"
+   "  foo,\n"
+   "  Bar{},\n"
+   "  [] {\n"
+   "auto g = h();\n"
+   "return g;\n"
+   "  },\n"
+   "  baz,\n"
+   "  qx);\n"
+   "}",
+   Style);
+  Style.BinPackArguments = true;
   Style.BreakBeforeBraces = FormatStyle::BS_Custom;
   Style.BraceWrapping.BeforeLambdaBody = true;
   verifyFormat("void foo() {\n"
"  aFunction(\n"
-   "  1, b(c(foo, Bar{}, baz,\n"
-   " [](d) -> Foo\n"
+   "  1, b(c(foo, Bar{}, baz, [](d) -> Foo\n"
   

[PATCH] D146101: [clang-format] Add BracedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

In D146101#4261293 , @MyDeveloperDay 
wrote:

> If all comments and concerns are done, then I'm inclined to accept, but I'd 
> like @owenpan  and @HazardyKnusperkeks to give their opinion before we land 
> this.

Sure. Thanks!




Comment at: clang/lib/Format/ContinuationIndenter.cpp:1665-1669
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? Style.ContinuationIndentWidth
+  : Style.DesignatedInitializerIndentWidth;
+  NewIndent = CurrentState.LastSpace + DesignatedInitializerIndentWidth;

owenpan wrote:
> HazardyKnusperkeks wrote:
> > MyDeveloperDay wrote:
> > > rymiel wrote:
> > > > owenpan wrote:
> > > > > jp4a50 wrote:
> > > > > > HazardyKnusperkeks wrote:
> > > > > > > owenpan wrote:
> > > > > > > > jp4a50 wrote:
> > > > > > > > > HazardyKnusperkeks wrote:
> > > > > > > > > > owenpan wrote:
> > > > > > > > > > > jp4a50 wrote:
> > > > > > > > > > > > owenpan wrote:
> > > > > > > > > > > > > owenpan wrote:
> > > > > > > > > > > > > > Using -1 to mean `ContinuationIndentWidth` is 
> > > > > > > > > > > > > > unnecessary and somewhat confusing IMO.
> > > > > > > > > > > > > Please disregard my comment above.
> > > > > > > > > > > > Just to make sure we are on the same page, does this 
> > > > > > > > > > > > mean that you are happy with the approach of using `-1` 
> > > > > > > > > > > > as a default value to indicate that 
> > > > > > > > > > > > `ContinuationIndentWidth` should be used?
> > > > > > > > > > > > 
> > > > > > > > > > > > I did initially consider using 
> > > > > > > > > > > > `std::optional` and using empty optional to 
> > > > > > > > > > > > indicate that `ContinuationIndentWidth` should be used 
> > > > > > > > > > > > but I saw that `PPIndentWidth` was using `-1` to 
> > > > > > > > > > > > default to using `IndentWidth` so I followed that 
> > > > > > > > > > > > precedent.
> > > > > > > > > > > Yep! I would prefer the `optional`, but as you pointed 
> > > > > > > > > > > out, we already got `PPIndentWidth`using `-1`.
> > > > > > > > > > From the C++ side I totally agree. One could use 
> > > > > > > > > > `value_or()`, which would make the code much more readable.
> > > > > > > > > > And just because `PPIndentWidth` is using -1 is no reason 
> > > > > > > > > > to repeat that, we could just as easily change 
> > > > > > > > > > `PPIndentWidht` to an optional.
> > > > > > > > > > 
> > > > > > > > > > But how would it look in yaml?
> > > > > > > > > In YAML we wouldn't need to support empty optional being 
> > > > > > > > > *explicitly* specified - it would just be the default.
> > > > > > > > > 
> > > > > > > > > So specifying `DesignatedInitializerIndentWidth: 4` would set 
> > > > > > > > > the `std::optional` to `4` but if 
> > > > > > > > > `DesignatedInitializerIndentWidth` was omitted from the YAML 
> > > > > > > > > then the optional would simply not be set during parsing.
> > > > > > > > > 
> > > > > > > > > Of course, if we were to change `PPIndentWidth` to work that 
> > > > > > > > > way too, it would technically be a breaking change because 
> > > > > > > > > users may have *explicitly* specified `-1` in their YAML.
> > > > > > > > > And just because `PPIndentWidth` is using -1 is no reason to 
> > > > > > > > > repeat that, we could just as easily change `PPIndentWidht` 
> > > > > > > > > to an optional.
> > > > > > > > 
> > > > > > > > We would have to deal with backward compatibility to avoid 
> > > > > > > > regressions though.
> > > > > > > > In YAML we wouldn't need to support empty optional being 
> > > > > > > > *explicitly* specified - it would just be the default.
> > > > > > > > 
> > > > > > > > So specifying `DesignatedInitializerIndentWidth: 4` would set 
> > > > > > > > the `std::optional` to `4` but if 
> > > > > > > > `DesignatedInitializerIndentWidth` was omitted from the YAML 
> > > > > > > > then the optional would simply not be set during parsing.
> > > > > > > > 
> > > > > > > > Of course, if we were to change `PPIndentWidth` to work that 
> > > > > > > > way too, it would technically be a breaking change because 
> > > > > > > > users may have *explicitly* specified `-1` in their YAML.
> > > > > > > 
> > > > > > > You need an explicit entry, because you need to be able to write 
> > > > > > > the empty optional on `--dump-config`.
> > > > > > > > In YAML we wouldn't need to support empty optional being 
> > > > > > > > *explicitly* specified - it would just be the default.
> > > > > > > > 
> > > > > > > > So specifying `DesignatedInitializerIndentWidth: 4` would set 
> > > > > > > > the `std::optional` to `4` but if 
> > > > > > > > `DesignatedInitializerIndentWidth` was omitted from the YAML 
> > > > > > > > then the optional would simply not be set during parsing.
> > > > > > > > 
> > > > > > > > Of course, if we were to change `PPIndentWidth` to work that 
> > > 

[PATCH] D146101: [clang-format] Add BracedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

Ping for review again plz.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146101: [clang-format] Add BracedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 marked 29 inline comments as done.
jp4a50 added a comment.

Marked comments as done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146101: [clang-format] Add BracedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 marked 4 inline comments as done.
jp4a50 added a comment.

Just FYI this is ready for review again. I believe I've addressed all comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146101: [clang-format] Add BracedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

Following on from our discussion about new options needing motivation from 
public style guides, I've updated the KJ style guide to clarify its stance on 
braced init lists: 
https://github.com/capnproto/capnproto/blob/master/style-guide.md#spacing-and-bracing


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

Thanks for accepting!

Commit details as follows as per previous diff:

Name: Jon Phillips
Email: jonap2...@gmail.com

If someone could commit for me that would be much appreciated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146042/new/

https://reviews.llvm.org/D146042

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

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 511098.
jp4a50 added a comment.

Add release note.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146042/new/

https://reviews.llvm.org/D146042

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22009,9 +22009,9 @@
   verifyFormat("void test() {\n"
"  (\n"
"  []() -> auto {\n"
-   "int b = 32;\n"
-   "return 3;\n"
-   "  },\n"
+   "int b = 32;\n"
+   "return 3;\n"
+   "  },\n"
"  foo, bar)\n"
"  .foo();\n"
"}",
@@ -22025,17 +22025,82 @@
"  .bar();\n"
"}",
Style);
-  Style = getGoogleStyle();
-  Style.LambdaBodyIndentation = FormatStyle::LBI_OuterScope;
-  verifyFormat("#define A   \\\n"
-   "  [] {  \\\n"
-   "xxx(\\\n"
-   "x); \\\n"
-   "  }",
-   Style);
-  // TODO: The current formatting has a minor issue that's not worth fixing
-  // right now whereby the closing brace is indented relative to the signature
-  // instead of being aligned. This only happens with macros.
+  verifyFormat("#define A  \\\n"
+   "  [] { \\\n"
+   "xxx(   \\\n"
+   "x);\\\n"
+   "  }",
+   Style);
+  verifyFormat("void foo() {\n"
+   "  aFunction(1, b(c(foo, bar, baz, [](d) {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  })));\n"
+   "}",
+   Style);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  verifyFormat("void foo() {\n"
+   "  aFunction(\n"
+   "  1, b(c(\n"
+   " [](d) -> Foo {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  },\n"
+   " foo, Bar{},\n"
+   " [] {\n"
+   "auto g = h();\n"
+   "return g;\n"
+   "  },\n"
+   " baz)));\n"
+   "}",
+   Style);
+  verifyFormat("void foo() {\n"
+   "  aFunction(1, b(c(foo, Bar{}, baz, [](d) -> Foo {\n"
+   "auto f = e(\n"
+   "foo,\n"
+   "[&] {\n"
+   "  auto g = h();\n"
+   "  return g;\n"
+   "},\n"
+   "qux,\n"
+   "[&] -> Bar {\n"
+   "  auto i = j();\n"
+   "  return i;\n"
+   "});\n"
+   "return f;\n"
+   "  })));\n"
+   "}",
+   Style);
+  verifyFormat("Namespace::Foo::Foo(\n"
+   "LongClassName bar, AnotherLongClassName baz)\n"
+   ": baz{baz}, func{[&] {\n"
+   "  auto qux = bar;\n"
+   "  return aFunkyFunctionCall(qux);\n"
+   "}} {}",
+   Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.BeforeLambdaBody = true;
+  verifyFormat("void foo() {\n"
+   "  aFunction(\n"
+   "  1, b(c(foo, Bar{}, baz,\n"
+   " [](d) -> Foo\n"
+   "  {\n"
+   "auto f = e(\n"
+   "[&]\n"
+   "{\n"
+   "  auto g = h();\n"
+   "  return g;\n"
+   "},\n"
+   "qux,\n"
+   "[&] -> Bar\n"
+   "{\n"
+   "  auto i = j();\n"
+   "  return i;\n"
+   "});\n"
+   "return f;\n"
+   "  })));\n"
+   "}",
+   Style);
 }
 
 TEST_F(FormatTest, LambdaWithLineComments) {
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -1003,17 

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 marked 2 inline comments as done.
jp4a50 added a comment.

In D146042#4239182 , @owenpan wrote:

> In D146042#4204651 , @jp4a50 wrote:
>
>> I'm confident that the patch will indent all those examples correctly when 
>> they are at block scope which is the only place those snippets will actually 
>> be valid code. I added an exception (see comment in 
>> ContinuationIndenter.cpp) to the code to disable `OuterScope`'s behaviour 
>> when the line's indentation level is 0 (i.e. statements at namespace scope) 
>> because otherwise you can end up with things like:
>>
>>   Namespace::Foo::Foo(
>> Arg arg1, Arg arg2,
>> Arg arg3, Arg arg4)
>> : init1{arg1},
>>   init2{[arg2]() {
>> return arg2;
>>   }},
>>   init3{arg3},
>>   init4{arg4} {}
>
> Sorry that I missed it.

NP - as I say, I've removed this special casing for now since it is 
controversial to include it in a patch claiming to simply fix bugs.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146042/new/

https://reviews.llvm.org/D146042

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

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 510901.
jp4a50 added a comment.

Tidy up docs and tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146042/new/

https://reviews.llvm.org/D146042

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22009,9 +22009,9 @@
   verifyFormat("void test() {\n"
"  (\n"
"  []() -> auto {\n"
-   "int b = 32;\n"
-   "return 3;\n"
-   "  },\n"
+   "int b = 32;\n"
+   "return 3;\n"
+   "  },\n"
"  foo, bar)\n"
"  .foo();\n"
"}",
@@ -22025,17 +22025,82 @@
"  .bar();\n"
"}",
Style);
-  Style = getGoogleStyle();
-  Style.LambdaBodyIndentation = FormatStyle::LBI_OuterScope;
-  verifyFormat("#define A   \\\n"
-   "  [] {  \\\n"
-   "xxx(\\\n"
-   "x); \\\n"
-   "  }",
-   Style);
-  // TODO: The current formatting has a minor issue that's not worth fixing
-  // right now whereby the closing brace is indented relative to the signature
-  // instead of being aligned. This only happens with macros.
+  verifyFormat("#define A  \\\n"
+   "  [] { \\\n"
+   "xxx(   \\\n"
+   "x);\\\n"
+   "  }",
+   Style);
+  verifyFormat("void foo() {\n"
+   "  aFunction(1, b(c(foo, bar, baz, [](d) {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  })));\n"
+   "}",
+   Style);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  verifyFormat("void foo() {\n"
+   "  aFunction(\n"
+   "  1, b(c(\n"
+   " [](d) -> Foo {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  },\n"
+   " foo, Bar{},\n"
+   " [] {\n"
+   "auto g = h();\n"
+   "return g;\n"
+   "  },\n"
+   " baz)));\n"
+   "}",
+   Style);
+  verifyFormat("void foo() {\n"
+   "  aFunction(1, b(c(foo, Bar{}, baz, [](d) -> Foo {\n"
+   "auto f = e(\n"
+   "foo,\n"
+   "[&] {\n"
+   "  auto g = h();\n"
+   "  return g;\n"
+   "},\n"
+   "qux,\n"
+   "[&] -> Bar {\n"
+   "  auto i = j();\n"
+   "  return i;\n"
+   "});\n"
+   "return f;\n"
+   "  })));\n"
+   "}",
+   Style);
+  verifyFormat("Namespace::Foo::Foo(\n"
+   "LongClassName bar, AnotherLongClassName baz)\n"
+   ": baz{baz}, func{[&] {\n"
+   "  auto qux = bar;\n"
+   "  return aFunkyFunctionCall(qux);\n"
+   "}} {}",
+   Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.BeforeLambdaBody = true;
+  verifyFormat("void foo() {\n"
+   "  aFunction(\n"
+   "  1, b(c(foo, Bar{}, baz,\n"
+   " [](d) -> Foo\n"
+   "  {\n"
+   "auto f = e(\n"
+   "[&]\n"
+   "{\n"
+   "  auto g = h();\n"
+   "  return g;\n"
+   "},\n"
+   "qux,\n"
+   "[&] -> Bar\n"
+   "{\n"
+   "  auto i = j();\n"
+   "  return i;\n"
+   "});\n"
+   "return f;\n"
+   "  })));\n"
+   "}",
+   Style);
 }
 
 TEST_F(FormatTest, LambdaWithLineComments) {
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -1003,17 +1003,6 @@
 
   int 

[PATCH] D146101: [clang-format] Add BracedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/include/clang/Format/Format.h:949
+  /// If unset, ``ContinuationIndentWidth`` is used.
+  /// \code
+  ///   AlignAfterOpenBracket: AlwaysBreak

owenpan wrote:
> HazardyKnusperkeks wrote:
> > jp4a50 wrote:
> > > MyDeveloperDay wrote:
> > > > did you check generating the html from the rst? I can never remember if 
> > > > we need a newline before the \code
> > > Nope - how do I do that exactly? I would guess a newline is not needed 
> > > based on other examples.
> > > did you check generating the html from the rst? I can never remember if 
> > > we need a newline before the \code
> > 
> > 
> > Nope - how do I do that exactly? I would guess a newline is not needed 
> > based on other examples.
> 
> See D147327#4236718.
HTML looks good.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1659
   opensProtoMessageField(Current, Style)) {
+const FormatToken *NextNonComment = Current.getNextNonComment();
 if (Current.opensBlockOrBlockTypeList(Style)) {

HazardyKnusperkeks wrote:
> Why did you move it?
I originally needed it in the new `else if` block since I was checking if it 
was a designated initializer period but the condition has since changed. Thanks 
for spotting. I've moved it back.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1664-1668
+  const auto BracedInitializerIndentWidth =
+  Style.BracedInitializerIndentWidth
+  ? *Style.BracedInitializerIndentWidth
+  : Style.ContinuationIndentWidth;
+  NewIndent = CurrentState.LastSpace + BracedInitializerIndentWidth;

HazardyKnusperkeks wrote:
> You can keep the local variable if you want, but please use `value_or`, it 
> expresses the intent better.
Have used `.value_or()` and got rid of temp variable since it's more concise 
now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146101: [clang-format] Add BracedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 510886.
jp4a50 added a comment.

Minor review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/docs/tools/dump_format_style.py
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -4855,6 +4855,191 @@
"[5] = ee};");
 }
 
+TEST_F(FormatTest, BracedInitializerIndentWidth) {
+  auto Style = getLLVMStyleWithColumns(60);
+  Style.BinPackArguments = true;
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BracedInitializerIndentWidth = 6;
+
+  // Non-initializing braces are unaffected by BracedInitializerIndentWidth.
+  verifyFormat("enum class {\n"
+   "  One,\n"
+   "  Two,\n"
+   "};\n",
+   Style);
+  verifyFormat("class Foo {\n"
+   "  Foo() {}\n"
+   "  void bar();\n"
+   "};\n",
+   Style);
+  verifyFormat("void foo() {\n"
+   "  auto bar = baz;\n"
+   "  return baz;\n"
+   "};\n",
+   Style);
+  verifyFormat("auto foo = [&] {\n"
+   "  auto bar = baz;\n"
+   "  return baz;\n"
+   "};\n",
+   Style);
+  verifyFormat("{\n"
+   "  auto bar = baz;\n"
+   "  return baz;\n"
+   "};\n",
+   Style);
+  // Non-brace initialization is unaffected by BracedInitializerIndentWidth.
+  verifyFormat("SomeClass clazz(\n"
+   "\"xx\", \"yy\",\n"
+   "\"zz\");\n",
+   Style);
+
+  // The following types of initialization are all affected by
+  // BracedInitializerIndentWidth. Aggregate initialization.
+  verifyFormat("int LongVariable[2] = {\n"
+   "  1000, 2000};",
+   Style);
+  verifyFormat("SomeStruct s{\n"
+   "  \"\", \"\",\n"
+   "  \"\"};\n",
+   Style);
+  // Designated initializers.
+  verifyFormat("int LongVariable[1] = {\n"
+   "  [0] = 1000, [1] = 2000};",
+   Style);
+  verifyFormat("SomeStruct s{\n"
+   "  .foo = \"x\",\n"
+   "  .bar = \"y\",\n"
+   "  .baz = \"z\"};\n",
+   Style);
+  // List initialization.
+  verifyFormat("SomeStruct s{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  verifyFormat("SomeStruct{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  verifyFormat("new SomeStruct{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  // Member initializer.
+  verifyFormat("class SomeClass {\n"
+   "  SomeStruct s{\n"
+   "\"x\",\n"
+   "\"y\",\n"
+   "\"z\",\n"
+   "  };\n"
+   "};\n",
+   Style);
+  // Constructor member initializer.
+  verifyFormat("SomeClass::SomeClass : strct{\n"
+   " \"x\",\n"
+   " \"y\",\n"
+   " \"z\",\n"
+   "   } {}\n",
+   Style);
+  // Copy initialization.
+  verifyFormat("SomeStruct s = SomeStruct{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  // Copy list initialization.
+  verifyFormat("SomeStruct s = {\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  // Assignment operand initialization.
+  verifyFormat("s = {\n"
+   "  \"x\",\n"
+   " 

[PATCH] D147556: [clang-format] Add BracedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 abandoned this revision.
jp4a50 added a comment.

Sorry - this is a duplicate of https://reviews.llvm.org/D146101 created by 
accident. Closing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147556/new/

https://reviews.llvm.org/D147556

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

[PATCH] D147556: [clang-format] Add BracedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 created this revision.
Herald added projects: All, clang, clang-format.
Herald added a subscriber: cfe-commits.
Herald added reviewers: rymiel, HazardyKnusperkeks, owenpan, MyDeveloperDay.
jp4a50 requested review of this revision.

The option allows users to specify how many columns to use to indent
the contents of braced init lists.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147556

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/docs/tools/dump_format_style.py
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -4855,6 +4855,191 @@
"[5] = ee};");
 }
 
+TEST_F(FormatTest, BracedInitializerIndentWidth) {
+  auto Style = getLLVMStyleWithColumns(60);
+  Style.BinPackArguments = true;
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BracedInitializerIndentWidth = 6;
+
+  // Non-initializing braces are unaffected by BracedInitializerIndentWidth.
+  verifyFormat("enum class {\n"
+   "  One,\n"
+   "  Two,\n"
+   "};\n",
+   Style);
+  verifyFormat("class Foo {\n"
+   "  Foo() {}\n"
+   "  void bar();\n"
+   "};\n",
+   Style);
+  verifyFormat("void foo() {\n"
+   "  auto bar = baz;\n"
+   "  return baz;\n"
+   "};\n",
+   Style);
+  verifyFormat("auto foo = [&] {\n"
+   "  auto bar = baz;\n"
+   "  return baz;\n"
+   "};\n",
+   Style);
+  verifyFormat("{\n"
+   "  auto bar = baz;\n"
+   "  return baz;\n"
+   "};\n",
+   Style);
+  // Non-brace initialization is unaffected by BracedInitializerIndentWidth.
+  verifyFormat("SomeClass clazz(\n"
+   "\"xx\", \"yy\",\n"
+   "\"zz\");\n",
+   Style);
+
+  // The following types of initialization are all affected by
+  // BracedInitializerIndentWidth. Aggregate initialization.
+  verifyFormat("int LongVariable[2] = {\n"
+   "  1000, 2000};",
+   Style);
+  verifyFormat("SomeStruct s{\n"
+   "  \"\", \"\",\n"
+   "  \"\"};\n",
+   Style);
+  // Designated initializers.
+  verifyFormat("int LongVariable[1] = {\n"
+   "  [0] = 1000, [1] = 2000};",
+   Style);
+  verifyFormat("SomeStruct s{\n"
+   "  .foo = \"x\",\n"
+   "  .bar = \"y\",\n"
+   "  .baz = \"z\"};\n",
+   Style);
+  // List initialization.
+  verifyFormat("SomeStruct s{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  verifyFormat("SomeStruct{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  verifyFormat("new SomeStruct{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  // Member initializer.
+  verifyFormat("class SomeClass {\n"
+   "  SomeStruct s{\n"
+   "\"x\",\n"
+   "\"y\",\n"
+   "\"z\",\n"
+   "  };\n"
+   "};\n",
+   Style);
+  // Constructor member initializer.
+  verifyFormat("SomeClass::SomeClass : strct{\n"
+   " \"x\",\n"
+   " \"y\",\n"
+   " \"z\",\n"
+   "   } {}\n",
+   Style);
+  // Copy initialization.
+  verifyFormat("SomeStruct s = SomeStruct{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  // Copy list initialization.
+  verifyFormat("SomeStruct s = {\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

Could I get a re-review on this one please?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146042/new/

https://reviews.llvm.org/D146042

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

[PATCH] D146101: [clang-format] Add BracedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 510022.
jp4a50 added a comment.

Alphabetical ordering.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/docs/tools/dump_format_style.py
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -4855,6 +4855,191 @@
"[5] = ee};");
 }
 
+TEST_F(FormatTest, BracedInitializerIndentWidth) {
+  auto Style = getLLVMStyleWithColumns(60);
+  Style.BinPackArguments = true;
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BracedInitializerIndentWidth = 6;
+
+  // Non-initializing braces are unaffected by BracedInitializerIndentWidth.
+  verifyFormat("enum class {\n"
+   "  One,\n"
+   "  Two,\n"
+   "};\n",
+   Style);
+  verifyFormat("class Foo {\n"
+   "  Foo() {}\n"
+   "  void bar();\n"
+   "};\n",
+   Style);
+  verifyFormat("void foo() {\n"
+   "  auto bar = baz;\n"
+   "  return baz;\n"
+   "};\n",
+   Style);
+  verifyFormat("auto foo = [&] {\n"
+   "  auto bar = baz;\n"
+   "  return baz;\n"
+   "};\n",
+   Style);
+  verifyFormat("{\n"
+   "  auto bar = baz;\n"
+   "  return baz;\n"
+   "};\n",
+   Style);
+  // Non-brace initialization is unaffected by BracedInitializerIndentWidth.
+  verifyFormat("SomeClass clazz(\n"
+   "\"xx\", \"yy\",\n"
+   "\"zz\");\n",
+   Style);
+
+  // The following types of initialization are all affected by
+  // BracedInitializerIndentWidth. Aggregate initialization.
+  verifyFormat("int LongVariable[2] = {\n"
+   "  1000, 2000};",
+   Style);
+  verifyFormat("SomeStruct s{\n"
+   "  \"\", \"\",\n"
+   "  \"\"};\n",
+   Style);
+  // Designated initializers.
+  verifyFormat("int LongVariable[1] = {\n"
+   "  [0] = 1000, [1] = 2000};",
+   Style);
+  verifyFormat("SomeStruct s{\n"
+   "  .foo = \"x\",\n"
+   "  .bar = \"y\",\n"
+   "  .baz = \"z\"};\n",
+   Style);
+  // List initialization.
+  verifyFormat("SomeStruct s{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  verifyFormat("SomeStruct{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  verifyFormat("new SomeStruct{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  // Member initializer.
+  verifyFormat("class SomeClass {\n"
+   "  SomeStruct s{\n"
+   "\"x\",\n"
+   "\"y\",\n"
+   "\"z\",\n"
+   "  };\n"
+   "};\n",
+   Style);
+  // Constructor member initializer.
+  verifyFormat("SomeClass::SomeClass : strct{\n"
+   " \"x\",\n"
+   " \"y\",\n"
+   " \"z\",\n"
+   "   } {}\n",
+   Style);
+  // Copy initialization.
+  verifyFormat("SomeStruct s = SomeStruct{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  // Copy list initialization.
+  verifyFormat("SomeStruct s = {\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  // Assignment operand initialization.
+  verifyFormat("s = {\n"
+   "  \"x\",\n"
+   " 

[PATCH] D146101: [clang-format] Add BracedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/include/clang/Format/Format.h:949
+  /// If unset, ``ContinuationIndentWidth`` is used.
+  /// \code
+  ///   AlignAfterOpenBracket: AlwaysBreak

MyDeveloperDay wrote:
> did you check generating the html from the rst? I can never remember if we 
> need a newline before the \code
Nope - how do I do that exactly? I would guess a newline is not needed based on 
other examples.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/docs/tools/dump_format_style.py:72
 
-  subtype, napplied = re.subn(r'^std::vector<(.*)>$', r'\1', typestr)
-  if napplied == 1:
-return 'List of ' + pluralize(to_yaml_type(subtype))
+  match = re.match(r'std::vector<(.*)>$', typestr)
+  if match:

I changed this from `subn` to `match` here since it's just a simpler way of 
expressing the same thing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

I've implemented BracedInitializerIndentWidth now with significantly extended 
test coverage (including checking that it is not applied *too* broadly). The 
actual implementation is just as simple as that for 
`DesignatedInitializerIndentWidth`.

> For the yaml stuff, I for one like to define everything (even it has the 
> default value), thus I'd like the -1 or something on output. But if that 
> leads to messing around with the yaml code just use what it does.

Pretty sure I would have to modify the YAML code in order to get it to output 
something when `std::optional` is set to `std::nullopt` so I have 
left it as-is.

PTAL.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 509982.
jp4a50 added a comment.

Replace DesignatedInitializerIndentWidth with BracedInitializerIndentWidth 
which applies to a broader range of initializers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/docs/tools/dump_format_style.py
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -4855,6 +4855,191 @@
"[5] = ee};");
 }
 
+TEST_F(FormatTest, BracedInitializerIndentWidth) {
+  auto Style = getLLVMStyleWithColumns(60);
+  Style.BinPackArguments = true;
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BracedInitializerIndentWidth = 6;
+
+  // Non-initializing braces are unaffected by BracedInitializerIndentWidth.
+  verifyFormat("enum class {\n"
+   "  One,\n"
+   "  Two,\n"
+   "};\n",
+   Style);
+  verifyFormat("class Foo {\n"
+   "  Foo() {}\n"
+   "  void bar();\n"
+   "};\n",
+   Style);
+  verifyFormat("void foo() {\n"
+   "  auto bar = baz;\n"
+   "  return baz;\n"
+   "};\n",
+   Style);
+  verifyFormat("auto foo = [&] {\n"
+   "  auto bar = baz;\n"
+   "  return baz;\n"
+   "};\n",
+   Style);
+  verifyFormat("{\n"
+   "  auto bar = baz;\n"
+   "  return baz;\n"
+   "};\n",
+   Style);
+  // Non-brace initialization is unaffected by BracedInitializerIndentWidth.
+  verifyFormat("SomeClass clazz(\n"
+   "\"xx\", \"yy\",\n"
+   "\"zz\");\n",
+   Style);
+
+  // The following types of initialization are all affected by
+  // BracedInitializerIndentWidth. Aggregate initialization.
+  verifyFormat("int LongVariable[2] = {\n"
+   "  1000, 2000};",
+   Style);
+  verifyFormat("SomeStruct s{\n"
+   "  \"\", \"\",\n"
+   "  \"\"};\n",
+   Style);
+  // Designated initializers.
+  verifyFormat("int LongVariable[1] = {\n"
+   "  [0] = 1000, [1] = 2000};",
+   Style);
+  verifyFormat("SomeStruct s{\n"
+   "  .foo = \"x\",\n"
+   "  .bar = \"y\",\n"
+   "  .baz = \"z\"};\n",
+   Style);
+  // List initialization.
+  verifyFormat("SomeStruct s{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  verifyFormat("SomeStruct{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  verifyFormat("new SomeStruct{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  // Member initializer.
+  verifyFormat("class SomeClass {\n"
+   "  SomeStruct s{\n"
+   "\"x\",\n"
+   "\"y\",\n"
+   "\"z\",\n"
+   "  };\n"
+   "};\n",
+   Style);
+  // Constructor member initializer.
+  verifyFormat("SomeClass::SomeClass : strct{\n"
+   " \"x\",\n"
+   " \"y\",\n"
+   " \"z\",\n"
+   "   } {}\n",
+   Style);
+  // Copy initialization.
+  verifyFormat("SomeStruct s = SomeStruct{\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  // Copy list initialization.
+  verifyFormat("SomeStruct s = {\n"
+   "  \"x\",\n"
+   "  \"y\",\n"
+   "  \"z\",\n"
+   "};\n",
+   Style);
+  // Assignment operand 

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

So at the risk of adding to the number of decisions we need to come to a 
consensus on, I was about to update the KJ style guide to explicitly call out 
the difference in indentation for designated initializers when I realized that 
we (both KJ code authors and clang-format contributors) should consider whether 
users should have the option to configure other similar types of indentation 
following opening braces.

I chatted to the owner of the KJ style guide and, whilst he did not have 
extremely strong opinions one way or another, he and I agreed that it probably 
makes more sense for such a config option to apply to other types of braced 
init lists.

Broadly speaking, these include aggregate initialization and list 
initialization (possibly direct initialization with braces too). See the 
initialization  
cppref article for links to all these.

As such, I would propose to actually rename `DesignatedInitializerIndentWidth` 
to `BracedInitializerIndentWidth` (but open to suggestiosn on exact naming) and 
have it apply to all the above types of initialization.

What does everyone think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1665-1669
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? Style.ContinuationIndentWidth
+  : Style.DesignatedInitializerIndentWidth;
+  NewIndent = CurrentState.LastSpace + DesignatedInitializerIndentWidth;

HazardyKnusperkeks wrote:
> owenpan wrote:
> > jp4a50 wrote:
> > > HazardyKnusperkeks wrote:
> > > > owenpan wrote:
> > > > > jp4a50 wrote:
> > > > > > owenpan wrote:
> > > > > > > owenpan wrote:
> > > > > > > > Using -1 to mean `ContinuationIndentWidth` is unnecessary and 
> > > > > > > > somewhat confusing IMO.
> > > > > > > Please disregard my comment above.
> > > > > > Just to make sure we are on the same page, does this mean that you 
> > > > > > are happy with the approach of using `-1` as a default value to 
> > > > > > indicate that `ContinuationIndentWidth` should be used?
> > > > > > 
> > > > > > I did initially consider using `std::optional` and using 
> > > > > > empty optional to indicate that `ContinuationIndentWidth` should be 
> > > > > > used but I saw that `PPIndentWidth` was using `-1` to default to 
> > > > > > using `IndentWidth` so I followed that precedent.
> > > > > Yep! I would prefer the `optional`, but as you pointed out, we 
> > > > > already got `PPIndentWidth`using `-1`.
> > > > From the C++ side I totally agree. One could use `value_or()`, which 
> > > > would make the code much more readable.
> > > > And just because `PPIndentWidth` is using -1 is no reason to repeat 
> > > > that, we could just as easily change `PPIndentWidht` to an optional.
> > > > 
> > > > But how would it look in yaml?
> > > In YAML we wouldn't need to support empty optional being *explicitly* 
> > > specified - it would just be the default.
> > > 
> > > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > > `std::optional` to `4` but if 
> > > `DesignatedInitializerIndentWidth` was omitted from the YAML then the 
> > > optional would simply not be set during parsing.
> > > 
> > > Of course, if we were to change `PPIndentWidth` to work that way too, it 
> > > would technically be a breaking change because users may have 
> > > *explicitly* specified `-1` in their YAML.
> > > And just because `PPIndentWidth` is using -1 is no reason to repeat that, 
> > > we could just as easily change `PPIndentWidht` to an optional.
> > 
> > We would have to deal with backward compatibility to avoid regressions 
> > though.
> > In YAML we wouldn't need to support empty optional being *explicitly* 
> > specified - it would just be the default.
> > 
> > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > `std::optional` to `4` but if `DesignatedInitializerIndentWidth` 
> > was omitted from the YAML then the optional would simply not be set during 
> > parsing.
> > 
> > Of course, if we were to change `PPIndentWidth` to work that way too, it 
> > would technically be a breaking change because users may have *explicitly* 
> > specified `-1` in their YAML.
> 
> You need an explicit entry, because you need to be able to write the empty 
> optional on `--dump-config`.
> > In YAML we wouldn't need to support empty optional being *explicitly* 
> > specified - it would just be the default.
> > 
> > So specifying `DesignatedInitializerIndentWidth: 4` would set the 
> > `std::optional` to `4` but if `DesignatedInitializerIndentWidth` 
> > was omitted from the YAML then the optional would simply not be set during 
> > parsing.
> > 
> > Of course, if we were to change `PPIndentWidth` to work that way too, it 
> > would technically be a breaking change because users may have *explicitly* 
> > specified `-1` in their YAML.
> 
> You need an explicit entry, because you need to be able to write the empty 
> optional on `--dump-config`.

It looks like the YAML IO logic just does the right thing (TM) with 
`std::optional`s. When calling `IO.mapOptional()` on output, it simply doesn't 
write the key out if the value is an empty optional. So I don't think this is 
an issue.

As @owenpan says, though, there is still the issue of backward compatibility 
with `PPIndentWidth`.

I don't feel strongly about which way to go so I'll leave it to you two to 
decide!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146995: [clang-format][NFC] Refactor unit tests for "LambdaBodyIndentation: OuterScope" option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

> You need to state a name and email for the commit.

Name: Jon Phillips
Email: jonap2...@gmail.com

Also, thanks for relating the child diff to this one. I couldn't see how to do 
it at first but have seen the option now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146995/new/

https://reviews.llvm.org/D146995

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

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1665-1669
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? Style.ContinuationIndentWidth
+  : Style.DesignatedInitializerIndentWidth;
+  NewIndent = CurrentState.LastSpace + DesignatedInitializerIndentWidth;

HazardyKnusperkeks wrote:
> owenpan wrote:
> > jp4a50 wrote:
> > > owenpan wrote:
> > > > owenpan wrote:
> > > > > Using -1 to mean `ContinuationIndentWidth` is unnecessary and 
> > > > > somewhat confusing IMO.
> > > > Please disregard my comment above.
> > > Just to make sure we are on the same page, does this mean that you are 
> > > happy with the approach of using `-1` as a default value to indicate that 
> > > `ContinuationIndentWidth` should be used?
> > > 
> > > I did initially consider using `std::optional` and using empty 
> > > optional to indicate that `ContinuationIndentWidth` should be used but I 
> > > saw that `PPIndentWidth` was using `-1` to default to using `IndentWidth` 
> > > so I followed that precedent.
> > Yep! I would prefer the `optional`, but as you pointed out, we already got 
> > `PPIndentWidth`using `-1`.
> From the C++ side I totally agree. One could use `value_or()`, which would 
> make the code much more readable.
> And just because `PPIndentWidth` is using -1 is no reason to repeat that, we 
> could just as easily change `PPIndentWidht` to an optional.
> 
> But how would it look in yaml?
In YAML we wouldn't need to support empty optional being *explicitly* specified 
- it would just be the default.

So specifying `DesignatedInitializerIndentWidth: 4` would set the 
`std::optional` to `4` but if `DesignatedInitializerIndentWidth` was 
omitted from the YAML then the optional would simply not be set during parsing.

Of course, if we were to change `PPIndentWidth` to work that way too, it would 
technically be a breaking change because users may have *explicitly* specified 
`-1` in their YAML.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146995: [clang-format][NFC] Refactor unit tests for "LambdaBodyIndentation: OuterScope" option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

If you guys are happy with this, could you please merge it for me?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146995/new/

https://reviews.llvm.org/D146995

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

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

I've removed the special-casing that I added for namespace scope statements. If 
I make those changes at all, I'll raise them as a follow up diff.




Comment at: clang/unittests/Format/FormatTest.cpp:21953
-  verifyFormat("std::sort(v.begin(), v.end(),\n"
-   "  [](const auto , const auto "
-   ") {\n"

MyDeveloperDay wrote:
> I would think the point here is to test someLongArgumentName not to use a 
> small variable like 'v'
These changes have been moved to D146995 now, but I'll respond here: the long 
variable names were (presumably) being used to cause a line break. Since I've 
reduced the column limit from 100 to 60, `v` is now sufficient (and less noisy 
to read).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146042/new/

https://reviews.llvm.org/D146042

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

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 508765.
jp4a50 added a comment.

Re-enable OuterScope lambda body indentation of lambdas in namespace scope 
statements.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146042/new/

https://reviews.llvm.org/D146042

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -21967,9 +21967,9 @@
   verifyFormat("void test() {\n"
"  (\n"
"  []() -> auto {\n"
-   "int b = 32;\n"
-   "return 3;\n"
-   "  },\n"
+   "int b = 32;\n"
+   "return 3;\n"
+   "  },\n"
"  foo, bar)\n"
"  .foo();\n"
"}",
@@ -21983,17 +21983,82 @@
"  .bar();\n"
"}",
Style);
-  Style = getGoogleStyle();
-  Style.LambdaBodyIndentation = FormatStyle::LBI_OuterScope;
-  verifyFormat("#define A   \\\n"
-   "  [] {  \\\n"
-   "xxx(\\\n"
-   "x); \\\n"
-   "  }",
-   Style);
-  // TODO: The current formatting has a minor issue that's not worth fixing
-  // right now whereby the closing brace is indented relative to the signature
-  // instead of being aligned. This only happens with macros.
+  verifyFormat("#define A  \\\n"
+   "  [] { \\\n"
+   "xxx(   \\\n"
+   "x);\\\n"
+   "  }",
+   Style);
+  verifyFormat("void foo() {\n"
+   "  aFunction(1, b(c(foo, bar, baz, [](d) {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  })));\n"
+   "}\n",
+   Style);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  verifyFormat("void foo() {\n"
+   "  aFunction(\n"
+   "  1, b(c(\n"
+   " [](d) -> Foo {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  },\n"
+   " foo, Bar{},\n"
+   " [] {\n"
+   "auto g = h();\n"
+   "return g;\n"
+   "  },\n"
+   " baz)));\n"
+   "}\n",
+   Style);
+  verifyFormat("void foo() {\n"
+   "  aFunction(1, b(c(foo, Bar{}, baz, [](d) -> Foo {\n"
+   "auto f = e(\n"
+   "foo,\n"
+   "[&] {\n"
+   "  auto g = h();\n"
+   "  return g;\n"
+   "},\n"
+   "qux,\n"
+   "[&] -> Bar {\n"
+   "  auto i = j();\n"
+   "  return i;\n"
+   "});\n"
+   "return f;\n"
+   "  })));\n"
+   "}\n",
+   Style);
+  verifyFormat("Namespace::Foo::Foo(\n"
+   "LongClassName bar, AnotherLongClassName baz)\n"
+   ": baz{baz}, func{[&] {\n"
+   "  auto qux = bar;\n"
+   "  return aFunkyFunctionCall(qux);\n"
+   "}} {}\n",
+   Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.BeforeLambdaBody = true;
+  verifyFormat("void foo() {\n"
+   "  aFunction(\n"
+   "  1, b(c(foo, Bar{}, baz,\n"
+   " [](d) -> Foo\n"
+   "  {\n"
+   "auto f = e(\n"
+   "[&]\n"
+   "{\n"
+   "  auto g = h();\n"
+   "  return g;\n"
+   "},\n"
+   "qux,\n"
+   "[&] -> Bar\n"
+   "{\n"
+   "  auto i = j();\n"
+   "  return i;\n"
+   "});\n"
+   "return f;\n"
+   "  })));\n"
+   "}\n",
+   Style);
 }
 
 TEST_F(FormatTest, LambdaWithLineComments) {
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ 

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:21952
"}",
Style);
+  verifyFormat("void test() {\n"

MyDeveloperDay wrote:
> I feel like lots of these test changes could be made on their own, BEFORE 
> changing the code, I would feel more comfortable about that I think
Pulled them out into D146995.



Comment at: clang/unittests/Format/FormatTest.cpp:21916
 
   // Lambdas with different indentation styles.
+  Style = getLLVMStyleWithColumns(60);

HazardyKnusperkeks wrote:
> jp4a50 wrote:
> > MyDeveloperDay wrote:
> > > jp4a50 wrote:
> > > > MyDeveloperDay wrote:
> > > > > Why are you changing existing tests
> > > > Sorry about the rather large diff here.
> > > > 
> > > > I have made the following changes to existing tests (all for what I 
> > > > think are good reasons):
> > > > 
> > > >   - Reduced the column limit and then reduced the verbosity of the code 
> > > > samples in the test to make them more readable. Before the column limit 
> > > > was 100 and the code samples were very long. I found this made them 
> > > > unnecessarily difficult to read, especially because some of the lines 
> > > > of the code sample would wrap before a newline in the code itself, 
> > > > which was confusing.
> > > >   - Changed a number of tests written in the form `EXPECT_EQ(code, 
> > > > format(otherCode))` to simply `verifyFormat(code)` because the latter 
> > > > is much shorter and easier to read. If you can think of a good reason 
> > > > to favour the former over the latter then let me know but it just 
> > > > seemed like unnecessary duplication to me.
> > > >   - Some of the tests weren't syntactically valid C++ e.g. the tests at 
> > > > line 21939 which I have changed to have a valid (rather than 
> > > > incomplete) trailing return type.
> > > >   - The macro test actually captured a bug so I had to change the code 
> > > > sample there to reflect the now fixed behaviour.
> > > > 
> > > > I also added one or two more tests at the bottom to capture more 
> > > > complex cases like when more than one argument to a function call is a 
> > > > lambda. 
> > > > 
> > > > 
> > > refactoring the tests is one thing, but not at the same time you are 
> > > modifying how they are formatted. Could it be a separate commit so we can 
> > > actually see what is changing?
> > > 
> > > I don't deny what is there before doesn't look 100% correct, but I've 
> > > always worked on the Beyoncé rule  (If you liked it you should have put a 
> > > test on it), meaning... someone put a test there to assert some form of 
> > > formatting behaviour, you are now changing that to be your desired 
> > > behaviour, that break the rule (IMHO)
> > > 
> > > we have to agree what was there before wasn't correct, but your changes 
> > > don't give me that confidence, 
> > > refactoring the tests is one thing, but not at the same time you are 
> > > modifying how they are formatted. Could it be a separate commit so we can 
> > > actually see what is changing?
> > 
> > Sure! I assume that this implies two separate Phabricator reviews/diffs? Or 
> > is there somehow a way to show two "commits" within a single review? Sorry, 
> > I'm not used to using Phrabricator.
> > 
> > > I don't deny what is there before doesn't look 100% correct, but I've 
> > > always worked on the Beyoncé rule  (If you liked it you should have put a 
> > > test on it), meaning... someone put a test there to assert some form of 
> > > formatting behaviour, you are now changing that to be your desired 
> > > behaviour, that break the rule (IMHO)
> > 
> > The macro example I was referring to as a "bug" actually had a comment 
> > below it which seemed to acknowledge an issue with the previous 
> > implementation. Now I'm not 100% sure that that comment was referring to 
> > the behaviour in the macro example but what does seem clear is that the 
> > behaviour shown in the macro test case is not desirable in the case of 
> > using `OuterScope`. I just can't see how that behaviour could be justified 
> > as being intentional/desirable. If you wanted, I could reach out to the 
> > original author of that test and ask them since they still work at the same 
> > company.
> > 
> > > we have to agree what was there before wasn't correct, but your changes 
> > > don't give me that confidence, 
> > 
> > I'd appreciate if you could elaborate on this - perhaps you're referring 
> > mainly to the macro example but I'm not sure.
> Each commit gets its own differential, there are no multiple commit reviews 
> like pull requests. You can add a parent or child commit to show the 
> dependency, if any.
Pulled out test refactoring (excluding functional changes) into D146995.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146042/new/

https://reviews.llvm.org/D146042

___

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 508762.
jp4a50 added a comment.

Rebase diff on top of D146995 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146042/new/

https://reviews.llvm.org/D146042

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -21959,17 +21959,19 @@
"  }\n"
"}",
Style);
-  verifyFormat("std::sort(v.begin(), v.end(),\n"
-   "  [](const auto , const auto ) {\n"
-   "  return foo.baz < bar.baz;\n"
-   "});\n",
+  verifyFormat("void test() {\n"
+   "  std::sort(v.begin(), v.end(),\n"
+   "[](const auto , const auto ) {\n"
+   "return foo.baz < bar.baz;\n"
+   "  });\n"
+   "}\n",
Style);
   verifyFormat("void test() {\n"
"  (\n"
"  []() -> auto {\n"
-   "int b = 32;\n"
-   "return 3;\n"
-   "  },\n"
+   "int b = 32;\n"
+   "return 3;\n"
+   "  },\n"
"  foo, bar)\n"
"  .foo();\n"
"}",
@@ -21983,17 +21985,82 @@
"  .bar();\n"
"}",
Style);
-  Style = getGoogleStyle();
-  Style.LambdaBodyIndentation = FormatStyle::LBI_OuterScope;
-  verifyFormat("#define A   \\\n"
-   "  [] {  \\\n"
-   "xxx(\\\n"
-   "x); \\\n"
-   "  }",
-   Style);
-  // TODO: The current formatting has a minor issue that's not worth fixing
-  // right now whereby the closing brace is indented relative to the signature
-  // instead of being aligned. This only happens with macros.
+  verifyFormat("#define A  \\\n"
+   "  [] { \\\n"
+   "xxx(   \\\n"
+   "x);\\\n"
+   "  }",
+   Style);
+  verifyFormat("void foo() {\n"
+   "  aFunction(1, b(c(foo, bar, baz, [](d) {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  })));\n"
+   "}\n",
+   Style);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  verifyFormat("void foo() {\n"
+   "  aFunction(\n"
+   "  1, b(c(\n"
+   " [](d) -> Foo {\n"
+   "auto f = e(d);\n"
+   "return f;\n"
+   "  },\n"
+   " foo, Bar{},\n"
+   " [] {\n"
+   "auto g = h();\n"
+   "return g;\n"
+   "  },\n"
+   " baz)));\n"
+   "}\n",
+   Style);
+  verifyFormat("void foo() {\n"
+   "  aFunction(1, b(c(foo, Bar{}, baz, [](d) -> Foo {\n"
+   "auto f = e(\n"
+   "foo,\n"
+   "[&] {\n"
+   "  auto g = h();\n"
+   "  return g;\n"
+   "},\n"
+   "qux,\n"
+   "[&] -> Bar {\n"
+   "  auto i = j();\n"
+   "  return i;\n"
+   "});\n"
+   "return f;\n"
+   "  })));\n"
+   "}\n",
+   Style);
+  verifyFormat("Namespace::Foo::Foo(\n"
+   "LongClassName bar, AnotherLongClassName baz)\n"
+   ": baz{baz}, func{[&] {\n"
+   "auto qux = bar;\n"
+   "return aFunkyFunctionCall(qux);\n"
+   "  }} {}\n",
+   Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.BeforeLambdaBody = true;
+  verifyFormat("void foo() {\n"
+   "  aFunction(\n"
+   "  1, b(c(foo, Bar{}, baz,\n"
+   " [](d) -> Foo\n"
+   "  {\n"
+   "auto f = e(\n"
+   "[&]\n"
+   "{\n"
+   "  auto g = h();\n"
+   "  return g;\n"
+   "},\n"
+   "  

[PATCH] D146995: [clang-format] Refactor unit tests for "LambdaBodyIndentation: OuterScope" option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:21978
Style);
-  EXPECT_EQ("SomeResult doSomething(SomeObject promise) {\n"
-"  return promise.then(\n"

The refactored version of these test cases has been moved up to immediately 
follow the setting of `OuterScope` so that the result can be compared with the 
identical code sample above which doesn't use `OuterScope`. It was odd that 
these weren't grouped together before.



Comment at: clang/unittests/Format/FormatTest.cpp:21994
+   Style);
   // TODO: The current formatting has a minor issue that's not worth fixing
   // right now whereby the closing brace is indented relative to the signature

This is the comment that I believe is calling out the seemingly broken 
formatting behaviour in the test case directly above it. I think it's pretty 
clear that the code in that test case should not be formatted that way when 
`OuterScope` is set (or even without it - the formatting just doesn't make any 
sense really).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146995/new/

https://reviews.llvm.org/D146995

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

[PATCH] D146995: [clang-format] Refactor unit tests for "LambdaBodyIndentation: OuterScope" option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 created this revision.
Herald added a project: All.
jp4a50 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The changes:

- make the tests more concise
- fix invalid C++ in the code samples
- ensure line breaks in tests' code samples correspond to line breaks in the 
test code itself for the avoidance of confusion


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146995

Files:
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -21914,51 +21914,59 @@
LLVMWithBeforeLambdaBody);
 
   // Lambdas with different indentation styles.
-  Style = getLLVMStyleWithColumns(100);
-  EXPECT_EQ("SomeResult doSomething(SomeObject promise) {\n"
-"  return promise.then(\n"
-"  [this, , someObject = "
-"std::mv(s)](std::vector evaluated) mutable {\n"
-"return someObject.startAsyncAction().then(\n"
-"[this, ](AsyncActionResult result) "
-"mutable { result.processMore(); });\n"
-"  });\n"
-"}\n",
-format("SomeResult doSomething(SomeObject promise) {\n"
-   "  return promise.then([this, , someObject = "
-   "std::mv(s)](std::vector evaluated) mutable {\n"
-   "return someObject.startAsyncAction().then([this, "
-   "](AsyncActionResult result) mutable {\n"
-   "  result.processMore();\n"
-   "});\n"
-   "  });\n"
-   "}\n",
-   Style));
+  Style = getLLVMStyleWithColumns(60);
+  verifyFormat("Result doSomething(Promise promise) {\n"
+   "  return promise.then(\n"
+   "  [this, obj = std::move(s)](int bar) mutable {\n"
+   "return someObject.startAsyncAction().then(\n"
+   "[this, ](Result result) mutable {\n"
+   "  result.processMore();\n"
+   "});\n"
+   "  });\n"
+   "}\n",
+   Style);
   Style.LambdaBodyIndentation = FormatStyle::LBI_OuterScope;
-  verifyFormat("test() {\n"
-   "  ([]() -> {\n"
+  verifyFormat("Result doSomething(Promise promise) {\n"
+   "  return promise.then(\n"
+   "  [this, obj = std::move(s)](int bar) mutable {\n"
+   "return obj.startAsyncAction().then(\n"
+   "[this, ](Result result) mutable {\n"
+   "  result.processMore();\n"
+   "});\n"
+   "  });\n"
+   "}\n",
+   Style);
+  verifyFormat("Result doSomething(Promise promise) {\n"
+   "  return promise.then([this, obj = std::move(s)] {\n"
+   "return obj.startAsyncAction().then(\n"
+   "[this, ](Result result) mutable {\n"
+   "  result.processMore();\n"
+   "});\n"
+   "  });\n"
+   "}\n",
+   Style);
+  verifyFormat("void test() {\n"
+   "  ([]() -> auto {\n"
"int b = 32;\n"
"return 3;\n"
"  }).foo();\n"
"}",
Style);
-  verifyFormat("test() {\n"
-   "  []() -> {\n"
+  verifyFormat("void test() {\n"
+   "  []() -> auto {\n"
"int b = 32;\n"
"return 3;\n"
"  }\n"
"}",
Style);
   verifyFormat("std::sort(v.begin(), v.end(),\n"
-   "  [](const auto , const auto "
-   ") {\n"
-   "  return someLongArgumentName.someMemberVariable < "
-   "someOtherLongArgumentName.someMemberVariable;\n"
-   "});",
+   "  [](const auto , const auto ) {\n"
+   "  return foo.baz < bar.baz;\n"
+   "});\n",
Style);
-  verifyFormat("test() {\n"
+  verifyFormat("void test() {\n"
"  (\n"
-   "  []() -> {\n"
+   "  []() -> auto {\n"
"int b = 32;\n"
"return 3;\n"
"  },\n"
@@ -21966,8 +21974,8 @@
"  .foo();\n"
"}",
Style);
-  verifyFormat("test() {\n"
-   "  ([]() -> {\n"
+  verifyFormat("void test() {\n"
+   "  ([]() -> auto {\n"
"int b = 32;\n"
"return 3;\n"
"  })\n"
@@ -21975,51 +21983,14 @@
"  .bar();\n"
"}",
Style);
-  EXPECT_EQ("SomeResult doSomething(SomeObject 

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 marked an inline comment as done.
jp4a50 added a comment.

All actionable comments have been addressed.




Comment at: clang/lib/Format/ContinuationIndenter.cpp:1665-1669
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? Style.ContinuationIndentWidth
+  : Style.DesignatedInitializerIndentWidth;
+  NewIndent = CurrentState.LastSpace + DesignatedInitializerIndentWidth;

owenpan wrote:
> owenpan wrote:
> > Using -1 to mean `ContinuationIndentWidth` is unnecessary and somewhat 
> > confusing IMO.
> Please disregard my comment above.
Just to make sure we are on the same page, does this mean that you are happy 
with the approach of using `-1` as a default value to indicate that 
`ContinuationIndentWidth` should be used?

I did initially consider using `std::optional` and using empty 
optional to indicate that `ContinuationIndentWidth` should be used but I saw 
that `PPIndentWidth` was using `-1` to default to using `IndentWidth` so I 
followed that precedent.



Comment at: clang/unittests/Format/FormatTest.cpp:4828
+   ".yy = 2,\n"
+   ".zz = 3};\n",
+   Style);

MyDeveloperDay wrote:
> jp4a50 wrote:
> > MyDeveloperDay wrote:
> > > Can we fix the brace positioning too
> > What would you expect the brace positioning to be here? I noticed that, 
> > with this configuration at least, the closing brace goes at the end of the 
> > same line (as the last initializer) when there is no trailing comma but 
> > goes on a new line if there is a trailing comma.
> part of me would like an option to not have to supply the trailing comma, 
I agree that it's slightly odd behaviour but I think it's pretty orthogonal to 
this change and would deserve it's own diff.

I think it's also pretty debatable what the behaviour *should* be. Should the 
`AlignAfterOpenBracket` option dictate the formatting of braces like this? 
Oddly enough, specifying `AlwaysBreak` (as I have done in this test) does 
indeed result in breaking after the `{` but changing it to `BlockIndent` 
doesn't put the closing `}` on a newline. Should we instead add a new 
`AlignAfterOpenBrace` analogue of `AlignAfterOpenBracket`?

Would you like me to to raise a separate issue to track this (if it doesn't 
already exist)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 508740.
jp4a50 added a comment.

Address minor review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -4820,6 +4820,34 @@
"[3] = cc,\n"
"[4] = dd,\n"
"[5] = ee};");
+
+  auto Style = getLLVMStyleWithColumns(60);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  verifyFormat("auto s = SomeStruct{\n"
+   ".xx = 1,\n"
+   ".yy = 2,\n"
+   ".zz = 3};\n",
+   Style);
+  Style.DesignatedInitializerIndentWidth = 2;
+  verifyFormat("auto s = SomeStruct{\n"
+   "  .xx = 1,\n"
+   "  .yy = 2,\n"
+   "  .zz = 3};\n",
+   Style);
+  verifyFormat("auto s = someFunctionCall(\n"
+   ", ,\n"
+   "SomeStruct{\n"
+   "  .xx = 1,\n"
+   "  .yy = 2,\n"
+   "  .zz = 3});\n",
+   Style);
+  Style.ContinuationIndentWidth = 8;
+  Style.DesignatedInitializerIndentWidth = -1; // Use ContinuationIndentWidth.
+  verifyFormat("auto s = SomeStruct{\n"
+   ".xx = 1,\n"
+   ".yy = 2,\n"
+   ".zz = 3};\n",
+   Style);
 }
 
 TEST_F(FormatTest, NestedStaticInitializers) {
Index: clang/unittests/Format/ConfigParseTest.cpp
===
--- clang/unittests/Format/ConfigParseTest.cpp
+++ clang/unittests/Format/ConfigParseTest.cpp
@@ -245,6 +245,10 @@
   SpacesBeforeTrailingComments, 1234u);
   CHECK_PARSE("IndentWidth: 32", IndentWidth, 32u);
   CHECK_PARSE("ContinuationIndentWidth: 11", ContinuationIndentWidth, 11u);
+  CHECK_PARSE("DesignatedInitializerIndentWidth: 34",
+  DesignatedInitializerIndentWidth, 34);
+  CHECK_PARSE("DesignatedInitializerIndentWidth: -1",
+  DesignatedInitializerIndentWidth, -1);
   CHECK_PARSE("CommentPragmas: '// abc$'", CommentPragmas, "// abc$");
 
   Style.QualifierAlignment = FormatStyle::QAS_Right;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -902,6 +902,8 @@
 IO.mapOptional("ContinuationIndentWidth", Style.ContinuationIndentWidth);
 IO.mapOptional("Cpp11BracedListStyle", Style.Cpp11BracedListStyle);
 IO.mapOptional("DerivePointerAlignment", Style.DerivePointerAlignment);
+IO.mapOptional("DesignatedInitializerIndentWidth",
+   Style.DesignatedInitializerIndentWidth);
 IO.mapOptional("DisableFormat", Style.DisableFormat);
 IO.mapOptional("EmptyLineAfterAccessModifier",
Style.EmptyLineAfterAccessModifier);
@@ -1367,6 +1369,7 @@
   LLVMStyle.ContinuationIndentWidth = 4;
   LLVMStyle.Cpp11BracedListStyle = true;
   LLVMStyle.DerivePointerAlignment = false;
+  LLVMStyle.DesignatedInitializerIndentWidth = -1;
   LLVMStyle.DisableFormat = false;
   LLVMStyle.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Never;
   LLVMStyle.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_LogicalBlock;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1656,13 +1656,20 @@
 CurrentState.NestedBlockIndent);
   if (Current.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
   opensProtoMessageField(Current, Style)) {
+const FormatToken *NextNoComment = Current.getNextNonComment();
 if (Current.opensBlockOrBlockTypeList(Style)) {
   NewIndent = Style.IndentWidth +
   std::min(State.Column, CurrentState.NestedBlockIndent);
+} else if (NextNoComment &&
+   NextNoComment->is(TT_DesignatedInitializerPeriod)) {
+  const auto DesignatedInitializerIndentWidth =
+  Style.DesignatedInitializerIndentWidth < 0
+  ? 

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:21918-21927
+  verifyFormat("Result doSomething(Promise promise) {\n"
+   "  return promise.then(\n"
+   "  [this, obj = std::move(s)](int evaluated) mutable {\n"
+   "return someObject.startAsyncAction().then(\n"
+   "[this, ](Result result) mutable {\n"
+   "  result.processMore();\n"
+   "});\n"

MyDeveloperDay wrote:
> This is a completely different function what happen to the std::vector 
> argument? 
The changes to shorten various types and variable names was to avoid lines (in 
the code sample) being broken onto multiple newlines where they weren't before 
because I changed the column limit from 100 to 60 (so leaving the code sample 
changed would have led to additional line breaks).

I changed the column limit from 100 to 60 purely because I thought the 100 
column limit made the tests unnecessarily verbose - there were lots of 
characters to read for seemingly no benefit.



Comment at: clang/unittests/Format/FormatTest.cpp:21916
 
   // Lambdas with different indentation styles.
+  Style = getLLVMStyleWithColumns(60);

MyDeveloperDay wrote:
> jp4a50 wrote:
> > MyDeveloperDay wrote:
> > > Why are you changing existing tests
> > Sorry about the rather large diff here.
> > 
> > I have made the following changes to existing tests (all for what I think 
> > are good reasons):
> > 
> >   - Reduced the column limit and then reduced the verbosity of the code 
> > samples in the test to make them more readable. Before the column limit was 
> > 100 and the code samples were very long. I found this made them 
> > unnecessarily difficult to read, especially because some of the lines of 
> > the code sample would wrap before a newline in the code itself, which was 
> > confusing.
> >   - Changed a number of tests written in the form `EXPECT_EQ(code, 
> > format(otherCode))` to simply `verifyFormat(code)` because the latter is 
> > much shorter and easier to read. If you can think of a good reason to 
> > favour the former over the latter then let me know but it just seemed like 
> > unnecessary duplication to me.
> >   - Some of the tests weren't syntactically valid C++ e.g. the tests at 
> > line 21939 which I have changed to have a valid (rather than incomplete) 
> > trailing return type.
> >   - The macro test actually captured a bug so I had to change the code 
> > sample there to reflect the now fixed behaviour.
> > 
> > I also added one or two more tests at the bottom to capture more complex 
> > cases like when more than one argument to a function call is a lambda. 
> > 
> > 
> refactoring the tests is one thing, but not at the same time you are 
> modifying how they are formatted. Could it be a separate commit so we can 
> actually see what is changing?
> 
> I don't deny what is there before doesn't look 100% correct, but I've always 
> worked on the Beyoncé rule  (If you liked it you should have put a test on 
> it), meaning... someone put a test there to assert some form of formatting 
> behaviour, you are now changing that to be your desired behaviour, that break 
> the rule (IMHO)
> 
> we have to agree what was there before wasn't correct, but your changes don't 
> give me that confidence, 
> refactoring the tests is one thing, but not at the same time you are 
> modifying how they are formatted. Could it be a separate commit so we can 
> actually see what is changing?

Sure! I assume that this implies two separate Phabricator reviews/diffs? Or is 
there somehow a way to show two "commits" within a single review? Sorry, I'm 
not used to using Phrabricator.

> I don't deny what is there before doesn't look 100% correct, but I've always 
> worked on the Beyoncé rule  (If you liked it you should have put a test on 
> it), meaning... someone put a test there to assert some form of formatting 
> behaviour, you are now changing that to be your desired behaviour, that break 
> the rule (IMHO)

The macro example I was referring to as a "bug" actually had a comment below it 
which seemed to acknowledge an issue with the previous implementation. Now I'm 
not 100% sure that that comment was referring to the behaviour in the macro 
example but what does seem clear is that the behaviour shown in the macro test 
case is not desirable in the case of using `OuterScope`. I just can't see how 
that behaviour could be justified as being intentional/desirable. If you 
wanted, I could reach out to the original author of that test and ask them 
since they still work at the same company.

> we have to agree what was there before wasn't correct, but your changes don't 
> give me that confidence, 

I'd appreciate if you could elaborate on this - perhaps you're referring mainly 
to the macro example but I'm not sure.


Repository:
  rG LLVM Github Monorepo

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

In D146101#4208962 , @MyDeveloperDay 
wrote:

> What I'd really like to see, is... in the event that 
> `ContinuationIndentWidth` is set and I do NOTset 
> `DesignatedInitializerIndentWidth`   , then 
> `DesignatedInitializerIndentWidth`  would inherit from that (not the default 
> as you have here),  if I set ContinuationIndentWidthto 3 
> DesignatedInitializerIndentWidth should be 3

Agree that this makes the most sense and, luckily, that is exactly the 
behaviour I implemented in my most recent diff!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

> The style guide doesn't mention indenting designated initializers with 2 
> spaces?

That is a fair point. I will get it updated because the author and maintainer 
of that style guide is the one requesting this change since, in practice, 
codebases following this style guide indent designated initializers with 2 
spaces.

>> I think it's also worth noting that the google style guide gives an example 
>> of designated initializers indented at 2 spaces (whereas their "continuation 
>> indent" for wrapped function parameters is 4).
>
> It's likely an error that the designated initializer example there shows 
> 2-space indents as clang-format uses the 4-space continuation indent width:

This is possible, but isn't it also possible that they would prefer designated 
initializers to be indented at 2 spaces but don't have the option with 
clang-format currently?

The only general information supplied in the google style guide about 
indentation is as follows:

> - Default indentation is 2 spaces.
> - Wrapped parameters have a 4 space indent.

If we are taking a strict definition of parameters, the above suggests to me 
that designated initializers would fall under the default indentation of 2 
spaces.

As mentioned on my other diff, I'm away until next Monday now so won't be able 
to get back to further comments til then.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

In D146042#4204513 , @owenpan wrote:

> Looks like this patch doesn't put the opening brace at the outerscope for 
> some of the examples from the linked github issues:
>
>   $ cat .clang-format
>   AllowShortLambdasOnASingleLine: None
>   BraceWrapping:
> BeforeLambdaBody: true
>   BreakBeforeBraces: Custom
>   LambdaBodyIndentation: OuterScope
>   $ cat test.cpp
>   (1,
>b(
>   []
>{
>  return 0;
>}));
>   
>   some_function(a_really_long_name, another_long_name, a_third_long_name,
> [&](LineCallback line_callback)
>   {
> int a;
> int b;
>   });
>   $ clang-format test.cpp
>   (1, b(
>   []
>   {
> return 0;
>   }));
>   
>   some_function(a_really_long_name, another_long_name, a_third_long_name,
> [&](LineCallback line_callback)
> {
>   int a;
>   int b;
> });
>   $ 
>
> Shouldn't the expected output be the same as the input?

I'm confident that the patch will indent all those examples correctly when they 
are at block scope which is the only place those snippets will actually be 
valid code. I added an exception (see comment in ContinuationIndenter.cpp) to 
the code to disable `OuterScope`'s behaviour when the line's indentation level 
is 0 (i.e. statements at namespace scope) because otherwise you can end up with 
things like:

  Namespace::Foo::Foo(
Arg arg1, Arg arg2,
Arg arg3, Arg arg4)
: init1{arg1},
  init2{[arg2]() {
return arg2;
  }},
  init3{arg3},
  init4{arg4} {}

The main rationale behind the `OuterScope` style is that the lambda body will 
be indented similarly to the code block of an `if` statement (or `for` etc) 
inside function definitions/blocks, but this breaks down when you have 
something like an constructor initializer which lives outside of a code block. 
The way I've handled this case isn't particularly elegant, though, so I'd be 
happy to remove the relevant code from this diff as it's not necessary to fix 
the issues I've linked - it was an extra bit of behaviour that my team (authors 
of the original implementation of `OuterScope`) wanted.

FYI I will be away until next Monday now but thanks for the feedback so far.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146042/new/

https://reviews.llvm.org/D146042

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

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

All comments addressed so bump on review please :)

Also, a build has failed 

 but I don't think my changes caused it - it looks like an old version of 
clang-format is being invoked which doesn't support a recently added option?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146042/new/

https://reviews.llvm.org/D146042

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

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

In D146101#4195293 , @owenpan wrote:

> Please see 
> https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options.
>  Is there a way to fix the issue without adding a new option?

@owenpan We could simply change the indentation level of designated 
initializers for everyone to match `IndentWidth` instead of 
`ContinuationIndentWidth` but I suspect that other users might be unhappy if we 
made that breaking change? I guess we could also consider making this behaviour 
only kick in when a specific set of *other* options are specified (e.g. 
`AlwaysBreak`) so that it applies to our clang-format style and not *all* 
others but that just feels like a hack.

I understand the added complexity and maintenance burden of a new option but we 
do meet the 3 criteria listed in your link.

- it is part of the KJ style guide which is used by the capn proto 
 project which has over 100 maintainers
- the style guide is publicly accessible here 

- I'm willing to contribute and maintain patches :)

I think it's also worth noting that the google style guide 
 
gives an example of designated initializers indented at 2 spaces (whereas their 
"continuation indent" for wrapped function parameters is 4).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 marked 2 inline comments as done.
jp4a50 added inline comments.



Comment at: clang/lib/Format/Format.cpp:1372
   LLVMStyle.DerivePointerAlignment = false;
+  LLVMStyle.DesignatedInitializerIndentWidth = 4;
   LLVMStyle.DisableFormat = false;

jp4a50 wrote:
> MyDeveloperDay wrote:
> > so lets say someone is using an IndentWidth of 2 and now you introduce this 
> > as being 4 here as the default
> > 
> > Without them changing anything, all their DesignatedIntializer code will 
> > get refactored to a IndentWidth of 4 rather than the 2 it was previously
> > 
> > This is where we get called out for "changing the defaults", which really 
> > we are not we are just reclassifying how it behaves.
> > 
> > What we normally say here is can you point us to a public style that has an 
> > independent DesignatedIntializerIndentWidth which is independent from the 
> > levels of IndentWidth everywhere else.
> > 
> > Whilst I can see more knobs feels good, this will change code and we'll 
> > have to manage that expectation.
> > 
> > 
> > 
> Yep, so as per my comment in `clang/docs/ClangFormatStyleOptions.rst` I think 
> I am changing my mind about this anyway.
> 
> My motivation for making this change is to support the [[ 
> https://github.com/capnproto/capnproto/blob/master/kjdoc/style-guide.md | KJ 
> style guide ]] which is quoted in the [[ 
> https://github.com/llvm/llvm-project/issues/51070 | github issue ]] - I work 
> on a team that uses the KJ style guide.
> 
> The KJ style guide wants a designated initializer indent width of 2 along 
> with a "normal" indent width of 2, so there is no explicit need for us to 
> have those two values be different.
> 
> When originally making these changes, I did think that having "more knobs" 
> was a good idea, but I agree that this could lead to annoying behaviour for 
> some users and they would probably expect the designated initializer indent 
> to match either the normal indent or the continuation indent.
> 
> How about I change the option to an integer and, when it's -1 (the default), 
> the designated initializer indent matches the continuation indent, but if it 
> is set to a value >= 0 then that number of columns is used instead? 
> 
I've reimplemented the option as a signed integer as per my suggestion above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 505540.
jp4a50 added a comment.

Change DesignatedInitializerIndentWidth to a signed integer which defaults to 
ContinuationIndentWidth when set to -1.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/ConfigParseTest.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -4820,6 +4820,34 @@
"[3] = cc,\n"
"[4] = dd,\n"
"[5] = ee};");
+
+  auto Style = getLLVMStyleWithColumns(60);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  verifyFormat("auto s = SomeStruct{\n"
+   ".xx = 1,\n"
+   ".yy = 2,\n"
+   ".zz = 3};\n",
+   Style);
+  Style.DesignatedInitializerIndentWidth = 2;
+  verifyFormat("auto s = SomeStruct{\n"
+   "  .xx = 1,\n"
+   "  .yy = 2,\n"
+   "  .zz = 3};\n",
+   Style);
+  verifyFormat("auto s = someFunctionCall(\n"
+   ", ,\n"
+   "SomeStruct{\n"
+   "  .xx = 1,\n"
+   "  .yy = 2,\n"
+   "  .zz = 3});\n",
+   Style);
+  Style.ContinuationIndentWidth = 8;
+  Style.DesignatedInitializerIndentWidth = -1; // Use ContinuationIndentWidth.
+  verifyFormat("auto s = SomeStruct{\n"
+   ".xx = 1,\n"
+   ".yy = 2,\n"
+   ".zz = 3};\n",
+   Style);
 }
 
 TEST_F(FormatTest, NestedStaticInitializers) {
Index: clang/unittests/Format/ConfigParseTest.cpp
===
--- clang/unittests/Format/ConfigParseTest.cpp
+++ clang/unittests/Format/ConfigParseTest.cpp
@@ -245,6 +245,10 @@
   SpacesBeforeTrailingComments, 1234u);
   CHECK_PARSE("IndentWidth: 32", IndentWidth, 32u);
   CHECK_PARSE("ContinuationIndentWidth: 11", ContinuationIndentWidth, 11u);
+  CHECK_PARSE("DesignatedInitializerIndentWidth: 34",
+  DesignatedInitializerIndentWidth, 34);
+  CHECK_PARSE("DesignatedInitializerIndentWidth: -1",
+  DesignatedInitializerIndentWidth, -1);
   CHECK_PARSE("CommentPragmas: '// abc$'", CommentPragmas, "// abc$");
 
   Style.QualifierAlignment = FormatStyle::QAS_Right;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -902,6 +902,8 @@
 IO.mapOptional("ContinuationIndentWidth", Style.ContinuationIndentWidth);
 IO.mapOptional("Cpp11BracedListStyle", Style.Cpp11BracedListStyle);
 IO.mapOptional("DerivePointerAlignment", Style.DerivePointerAlignment);
+IO.mapOptional("DesignatedInitializerIndentWidth",
+   Style.DesignatedInitializerIndentWidth);
 IO.mapOptional("DisableFormat", Style.DisableFormat);
 IO.mapOptional("EmptyLineAfterAccessModifier",
Style.EmptyLineAfterAccessModifier);
@@ -1367,6 +1369,7 @@
   LLVMStyle.ContinuationIndentWidth = 4;
   LLVMStyle.Cpp11BracedListStyle = true;
   LLVMStyle.DerivePointerAlignment = false;
+  LLVMStyle.DesignatedInitializerIndentWidth = -1;
   LLVMStyle.DisableFormat = false;
   LLVMStyle.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Never;
   LLVMStyle.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_LogicalBlock;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1656,13 +1656,20 @@
 CurrentState.NestedBlockIndent);
   if (Current.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
   opensProtoMessageField(Current, Style)) {
+const FormatToken *NextNoComment = Current.getNextNonComment();
 if (Current.opensBlockOrBlockTypeList(Style)) {
   NewIndent = Style.IndentWidth +
   std::min(State.Column, CurrentState.NestedBlockIndent);
+} else if (NextNoComment &&
+   NextNoComment->is(TT_DesignatedInitializerPeriod)) {
+  const auto DesignatedInitializerIndentWidth =
+  

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/lib/Format/Format.cpp:1372
   LLVMStyle.DerivePointerAlignment = false;
+  LLVMStyle.DesignatedInitializerIndentWidth = 4;
   LLVMStyle.DisableFormat = false;

MyDeveloperDay wrote:
> so lets say someone is using an IndentWidth of 2 and now you introduce this 
> as being 4 here as the default
> 
> Without them changing anything, all their DesignatedIntializer code will get 
> refactored to a IndentWidth of 4 rather than the 2 it was previously
> 
> This is where we get called out for "changing the defaults", which really we 
> are not we are just reclassifying how it behaves.
> 
> What we normally say here is can you point us to a public style that has an 
> independent DesignatedIntializerIndentWidth which is independent from the 
> levels of IndentWidth everywhere else.
> 
> Whilst I can see more knobs feels good, this will change code and we'll have 
> to manage that expectation.
> 
> 
> 
Yep, so as per my comment in `clang/docs/ClangFormatStyleOptions.rst` I think I 
am changing my mind about this anyway.

My motivation for making this change is to support the [[ 
https://github.com/capnproto/capnproto/blob/master/kjdoc/style-guide.md | KJ 
style guide ]] which is quoted in the [[ 
https://github.com/llvm/llvm-project/issues/51070 | github issue ]] - I work on 
a team that uses the KJ style guide.

The KJ style guide wants a designated initializer indent width of 2 along with 
a "normal" indent width of 2, so there is no explicit need for us to have those 
two values be different.

When originally making these changes, I did think that having "more knobs" was 
a good idea, but I agree that this could lead to annoying behaviour for some 
users and they would probably expect the designated initializer indent to match 
either the normal indent or the continuation indent.

How about I change the option to an integer and, when it's -1 (the default), 
the designated initializer indent matches the continuation indent, but if it is 
set to a value >= 0 then that number of columns is used instead? 




Comment at: clang/unittests/Format/FormatTest.cpp:4828
+   ".yy = 2,\n"
+   ".zz = 3};\n",
+   Style);

MyDeveloperDay wrote:
> Can we fix the brace positioning too
What would you expect the brace positioning to be here? I noticed that, with 
this configuration at least, the closing brace goes at the end of the same line 
(as the last initializer) when there is no trailing comma but goes on a new 
line if there is a trailing comma.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 505449.
jp4a50 added a comment.

Update OuterScope docs in Format.h and regenerate public docs.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146042/new/

https://reviews.llvm.org/D146042

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -21914,60 +21914,61 @@
LLVMWithBeforeLambdaBody);
 
   // Lambdas with different indentation styles.
-  Style = getLLVMStyleWithColumns(100);
-  EXPECT_EQ("SomeResult doSomething(SomeObject promise) {\n"
-"  return promise.then(\n"
-"  [this, , someObject = "
-"std::mv(s)](std::vector evaluated) mutable {\n"
-"return someObject.startAsyncAction().then(\n"
-"[this, ](AsyncActionResult result) "
-"mutable { result.processMore(); });\n"
-"  });\n"
-"}\n",
-format("SomeResult doSomething(SomeObject promise) {\n"
-   "  return promise.then([this, , someObject = "
-   "std::mv(s)](std::vector evaluated) mutable {\n"
-   "return someObject.startAsyncAction().then([this, "
-   "](AsyncActionResult result) mutable {\n"
-   "  result.processMore();\n"
-   "});\n"
-   "  });\n"
-   "}\n",
-   Style));
+  Style = getLLVMStyleWithColumns(60);
+  verifyFormat("Result doSomething(Promise promise) {\n"
+   "  return promise.then(\n"
+   "  [this, obj = std::move(s)](int evaluated) mutable {\n"
+   "return someObject.startAsyncAction().then(\n"
+   "[this, ](Result result) mutable {\n"
+   "  result.processMore();\n"
+   "});\n"
+   "  });\n"
+   "}\n",
+   Style);
   Style.LambdaBodyIndentation = FormatStyle::LBI_OuterScope;
-  verifyFormat("test() {\n"
-   "  ([]() -> {\n"
+  verifyFormat("Result doSomething(Promise promise) {\n"
+   "  return promise.then(\n"
+   "  [this, obj = std::move(s)](int bar) mutable {\n"
+   "return obj.startAsyncAction().then(\n"
+   "[this, ](Result result) mutable {\n"
+   "  result.processMore();\n"
+   "});\n"
+   "  });\n"
+   "}\n",
+   Style);
+  verifyFormat("void test() {\n"
+   "  ([]() -> auto {\n"
"int b = 32;\n"
"return 3;\n"
"  }).foo();\n"
"}",
Style);
-  verifyFormat("test() {\n"
-   "  []() -> {\n"
+  verifyFormat("void test() {\n"
+   "  []() -> auto {\n"
"int b = 32;\n"
"return 3;\n"
"  }\n"
"}",
Style);
-  verifyFormat("std::sort(v.begin(), v.end(),\n"
-   "  [](const auto , const auto "
-   ") {\n"
-   "  return someLongArgumentName.someMemberVariable < "
-   "someOtherLongArgumentName.someMemberVariable;\n"
-   "});",
+  verifyFormat("void test() {\n"
+   "  std::sort(v.begin(), v.end(),\n"
+   "[](const auto , const auto ) {\n"
+   "return foo.baz < bar.baz;\n"
+   "  });\n"
+   "}\n",
Style);
-  verifyFormat("test() {\n"
+  verifyFormat("void test() {\n"
"  (\n"
-   "  []() -> {\n"
-   "int b = 32;\n"
-   "return 3;\n"
-   "  },\n"
+   "  []() -> auto {\n"
+   "int b = 32;\n"
+   "return 3;\n"
+   "  },\n"
"  foo, bar)\n"
"  .foo();\n"
"}",
Style);
-  verifyFormat("test() {\n"
-   "  ([]() -> {\n"
+  verifyFormat("void test() {\n"
+   "  ([]() -> auto {\n"
"int b = 32;\n"
"return 3;\n"
"  })\n"
@@ -21975,54 +21976,82 @@
"  .bar();\n"
"}",
Style);
-  EXPECT_EQ("SomeResult doSomething(SomeObject promise) {\n"
-"  return promise.then(\n"
-"  [this, , someObject = "
-"std::mv(s)](std::vector evaluated) mutable {\n"
-"return 

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:3540
 
+   someMethod(someOtherMethod(
+   [](SomeReallyLongLambdaSignatureArgument foo) {

jp4a50 wrote:
> MyDeveloperDay wrote:
> > This code may come from Format.h
> Sorry, I'm not sure I understand.
> 
> Do you mean that this code should *also* be added to the corresponding 
> documentation in `Format.h`? Or do you mean that the documentation in this 
> file is programatically generated from `Format.h` and I should therefore only 
> put it there? Or something else?
Ahh I've found `dump_format_style.py` now. Will fix.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146042/new/

https://reviews.llvm.org/D146042

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

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:3540
 
+   someMethod(someOtherMethod(
+   [](SomeReallyLongLambdaSignatureArgument foo) {

MyDeveloperDay wrote:
> This code may come from Format.h
Sorry, I'm not sure I understand.

Do you mean that this code should *also* be added to the corresponding 
documentation in `Format.h`? Or do you mean that the documentation in this file 
is programatically generated from `Format.h` and I should therefore only put it 
there? Or something else?



Comment at: clang/unittests/Format/FormatTest.cpp:21916
 
   // Lambdas with different indentation styles.
+  Style = getLLVMStyleWithColumns(60);

MyDeveloperDay wrote:
> Why are you changing existing tests
Sorry about the rather large diff here.

I have made the following changes to existing tests (all for what I think are 
good reasons):

  - Reduced the column limit and then reduced the verbosity of the code samples 
in the test to make them more readable. Before the column limit was 100 and the 
code samples were very long. I found this made them unnecessarily difficult to 
read, especially because some of the lines of the code sample would wrap before 
a newline in the code itself, which was confusing.
  - Changed a number of tests written in the form `EXPECT_EQ(code, 
format(otherCode))` to simply `verifyFormat(code)` because the latter is much 
shorter and easier to read. If you can think of a good reason to favour the 
former over the latter then let me know but it just seemed like unnecessary 
duplication to me.
  - Some of the tests weren't syntactically valid C++ e.g. the tests at line 
21939 which I have changed to have a valid (rather than incomplete) trailing 
return type.
  - The macro test actually captured a bug so I had to change the code sample 
there to reflect the now fixed behaviour.

I also added one or two more tests at the bottom to capture more complex cases 
like when more than one argument to a function call is a lambda. 




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146042/new/

https://reviews.llvm.org/D146042

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

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added a comment.

In D146042#4195295 , @owenpan wrote:

>> The previous implementation of the option involved a hack which corrupted 
>> the parenthesis state stack.
>
> Can you link the review (e.g. `Dnn`) of the previous implementation in 
> the summary?
>
>> Specifically, this change fixes github issues #55708, #53212, #52846 and 
>> #59954.
>
> Please link the issues (e.g. 
> https://github.com/llvm/llvm-project/issues/n).

Done! Please see updated description.

To give a little more relevant context, my team were the original authors of 
this feature/option (primarily for our own benefit) so we are motivated to fix 
it and get it right this time.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146042/new/

https://reviews.llvm.org/D146042

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

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:2704
+
+**DesignatedInitializerIndentWidth** (``Unsigned``)
+  The number of columns to use to indent designated initializers that start on 
a new line.

Perhaps it would be better to make this a signed integer and default to `-1` 
which would cause `ContinuationIndentWidth` to be used (i.e. current behaviour)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

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

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 505302.
jp4a50 added a comment.

Apply clang-format.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146101/new/

https://reviews.llvm.org/D146101

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -4820,6 +4820,26 @@
"[3] = cc,\n"
"[4] = dd,\n"
"[5] = ee};");
+  auto Style = getLLVMStyleWithColumns(60);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  verifyFormat("auto s = SomeStruct{\n"
+   ".xx = 1,\n"
+   ".yy = 2,\n"
+   ".zz = 3};\n",
+   Style);
+  Style.DesignatedInitializerIndentWidth = 2;
+  verifyFormat("auto s = SomeStruct{\n"
+   "  .xx = 1,\n"
+   "  .yy = 2,\n"
+   "  .zz = 3};\n",
+   Style);
+  verifyFormat("auto s = someFunctionCall(\n"
+   ", ,\n"
+   "SomeStruct{\n"
+   "  .xx = 1,\n"
+   "  .yy = 2,\n"
+   "  .zz = 3});\n",
+   Style);
 }
 
 TEST_F(FormatTest, NestedStaticInitializers) {
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -902,6 +902,8 @@
 IO.mapOptional("ContinuationIndentWidth", Style.ContinuationIndentWidth);
 IO.mapOptional("Cpp11BracedListStyle", Style.Cpp11BracedListStyle);
 IO.mapOptional("DerivePointerAlignment", Style.DerivePointerAlignment);
+IO.mapOptional("DesignatedInitializerIndentWidth",
+   Style.DesignatedInitializerIndentWidth);
 IO.mapOptional("DisableFormat", Style.DisableFormat);
 IO.mapOptional("EmptyLineAfterAccessModifier",
Style.EmptyLineAfterAccessModifier);
@@ -1367,6 +1369,7 @@
   LLVMStyle.ContinuationIndentWidth = 4;
   LLVMStyle.Cpp11BracedListStyle = true;
   LLVMStyle.DerivePointerAlignment = false;
+  LLVMStyle.DesignatedInitializerIndentWidth = 4;
   LLVMStyle.DisableFormat = false;
   LLVMStyle.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Never;
   LLVMStyle.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_LogicalBlock;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1656,13 +1656,16 @@
 CurrentState.NestedBlockIndent);
   if (Current.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
   opensProtoMessageField(Current, Style)) {
+const FormatToken *NextNoComment = Current.getNextNonComment();
 if (Current.opensBlockOrBlockTypeList(Style)) {
   NewIndent = Style.IndentWidth +
   std::min(State.Column, CurrentState.NestedBlockIndent);
+} else if (NextNoComment->is(TT_DesignatedInitializerPeriod)) {
+  NewIndent =
+  CurrentState.LastSpace + Style.DesignatedInitializerIndentWidth;
 } else {
   NewIndent = CurrentState.LastSpace + Style.ContinuationIndentWidth;
 }
-const FormatToken *NextNoComment = Current.getNextNonComment();
 bool EndsInComma = Current.MatchingParen &&
Current.MatchingParen->Previous &&
Current.MatchingParen->Previous->is(tok::comma);
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -2020,6 +2020,17 @@
   /// \version 3.7
   bool DerivePointerAlignment;
 
+  /// The number of columns to use to indent designated initializers that start
+  /// on a new line. \code
+  ///   void f() {
+  /// auto s = SomeStruct{
+  ///   .foo = "foo",
+  ///   .bar = "bar",
+  ///   .baz = "baz",
+  /// };
+  ///   }
+  unsigned DesignatedInitializerIndentWidth;
+
   /// Disables formatting completely.
   /// \version 3.7
   bool DisableFormat;
@@ -4236,6 +4247,8 @@
ContinuationIndentWidth == R.ContinuationIndentWidth &&
Cpp11BracedListStyle == R.Cpp11BracedListStyle &&
DerivePointerAlignment == R.DerivePointerAlignment &&
+   

[PATCH] D146101: [clang-format] Add DesignatedInitializerIndentWidth option.

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 created this revision.
Herald added a project: All.
jp4a50 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The option allows users to specify how many columns to use to indent
designated initializers that start on a new line.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146101

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -4820,6 +4820,25 @@
"[3] = cc,\n"
"[4] = dd,\n"
"[5] = ee};");
+  auto Style = getLLVMStyleWithColumns(60);
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  verifyFormat("auto s = SomeStruct{\n"
+   ".xx = 1,\n"
+   ".yy = 2,\n"
+   ".zz = 3};\n",
+   Style);
+  Style.DesignatedInitializerIndentWidth = 2;
+  verifyFormat("auto s = SomeStruct{\n"
+   "  .xx = 1,\n"
+   "  .yy = 2,\n"
+   "  .zz = 3};\n",
+   Style);
+  verifyFormat("auto s = someFunctionCall(\n"
+   ", ,\n"
+   "SomeStruct{\n"
+   "  .xx = 1,\n"
+   "  .yy = 2,\n"
+   "  .zz = 3});\n", Style);
 }
 
 TEST_F(FormatTest, NestedStaticInitializers) {
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -902,6 +902,7 @@
 IO.mapOptional("ContinuationIndentWidth", Style.ContinuationIndentWidth);
 IO.mapOptional("Cpp11BracedListStyle", Style.Cpp11BracedListStyle);
 IO.mapOptional("DerivePointerAlignment", Style.DerivePointerAlignment);
+IO.mapOptional("DesignatedInitializerIndentWidth", Style.DesignatedInitializerIndentWidth);
 IO.mapOptional("DisableFormat", Style.DisableFormat);
 IO.mapOptional("EmptyLineAfterAccessModifier",
Style.EmptyLineAfterAccessModifier);
@@ -1367,6 +1368,7 @@
   LLVMStyle.ContinuationIndentWidth = 4;
   LLVMStyle.Cpp11BracedListStyle = true;
   LLVMStyle.DerivePointerAlignment = false;
+  LLVMStyle.DesignatedInitializerIndentWidth = 4;
   LLVMStyle.DisableFormat = false;
   LLVMStyle.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Never;
   LLVMStyle.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_LogicalBlock;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1656,13 +1656,15 @@
 CurrentState.NestedBlockIndent);
   if (Current.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
   opensProtoMessageField(Current, Style)) {
+const FormatToken *NextNoComment = Current.getNextNonComment();
 if (Current.opensBlockOrBlockTypeList(Style)) {
   NewIndent = Style.IndentWidth +
   std::min(State.Column, CurrentState.NestedBlockIndent);
+} else if (NextNoComment->is(TT_DesignatedInitializerPeriod)) {
+  NewIndent = CurrentState.LastSpace + Style.DesignatedInitializerIndentWidth;
 } else {
   NewIndent = CurrentState.LastSpace + Style.ContinuationIndentWidth;
 }
-const FormatToken *NextNoComment = Current.getNextNonComment();
 bool EndsInComma = Current.MatchingParen &&
Current.MatchingParen->Previous &&
Current.MatchingParen->Previous->is(tok::comma);
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -2020,6 +2020,17 @@
   /// \version 3.7
   bool DerivePointerAlignment;
 
+  /// The number of columns to use to indent designated initializers that start on a new line.
+  /// \code
+  ///   void f() {
+  /// auto s = SomeStruct{
+  ///   .foo = "foo",
+  ///   .bar = "bar",
+  ///   .baz = "baz",
+  /// };
+  ///   }
+  unsigned DesignatedInitializerIndentWidth;
+
   /// Disables formatting completely.
   /// \version 3.7
   bool DisableFormat;
@@ -4236,6 +4247,7 @@
ContinuationIndentWidth == R.ContinuationIndentWidth &&
Cpp11BracedListStyle == R.Cpp11BracedListStyle &&

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 updated this revision to Diff 505264.
jp4a50 added a comment.

Add new code sample to demonstrate behaviour of "LambdaBodyIndentation: 
OuterScope"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146042/new/

https://reviews.llvm.org/D146042

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -21914,60 +21914,61 @@
LLVMWithBeforeLambdaBody);
 
   // Lambdas with different indentation styles.
-  Style = getLLVMStyleWithColumns(100);
-  EXPECT_EQ("SomeResult doSomething(SomeObject promise) {\n"
-"  return promise.then(\n"
-"  [this, , someObject = "
-"std::mv(s)](std::vector evaluated) mutable {\n"
-"return someObject.startAsyncAction().then(\n"
-"[this, ](AsyncActionResult result) "
-"mutable { result.processMore(); });\n"
-"  });\n"
-"}\n",
-format("SomeResult doSomething(SomeObject promise) {\n"
-   "  return promise.then([this, , someObject = "
-   "std::mv(s)](std::vector evaluated) mutable {\n"
-   "return someObject.startAsyncAction().then([this, "
-   "](AsyncActionResult result) mutable {\n"
-   "  result.processMore();\n"
-   "});\n"
-   "  });\n"
-   "}\n",
-   Style));
+  Style = getLLVMStyleWithColumns(60);
+  verifyFormat("Result doSomething(Promise promise) {\n"
+   "  return promise.then(\n"
+   "  [this, obj = std::move(s)](int evaluated) mutable {\n"
+   "return someObject.startAsyncAction().then(\n"
+   "[this, ](Result result) mutable {\n"
+   "  result.processMore();\n"
+   "});\n"
+   "  });\n"
+   "}\n",
+   Style);
   Style.LambdaBodyIndentation = FormatStyle::LBI_OuterScope;
-  verifyFormat("test() {\n"
-   "  ([]() -> {\n"
+  verifyFormat("Result doSomething(Promise promise) {\n"
+   "  return promise.then(\n"
+   "  [this, obj = std::move(s)](int bar) mutable {\n"
+   "return obj.startAsyncAction().then(\n"
+   "[this, ](Result result) mutable {\n"
+   "  result.processMore();\n"
+   "});\n"
+   "  });\n"
+   "}\n",
+   Style);
+  verifyFormat("void test() {\n"
+   "  ([]() -> auto {\n"
"int b = 32;\n"
"return 3;\n"
"  }).foo();\n"
"}",
Style);
-  verifyFormat("test() {\n"
-   "  []() -> {\n"
+  verifyFormat("void test() {\n"
+   "  []() -> auto {\n"
"int b = 32;\n"
"return 3;\n"
"  }\n"
"}",
Style);
-  verifyFormat("std::sort(v.begin(), v.end(),\n"
-   "  [](const auto , const auto "
-   ") {\n"
-   "  return someLongArgumentName.someMemberVariable < "
-   "someOtherLongArgumentName.someMemberVariable;\n"
-   "});",
+  verifyFormat("void test() {\n"
+   "  std::sort(v.begin(), v.end(),\n"
+   "[](const auto , const auto ) {\n"
+   "return foo.baz < bar.baz;\n"
+   "  });\n"
+   "}\n",
Style);
-  verifyFormat("test() {\n"
+  verifyFormat("void test() {\n"
"  (\n"
-   "  []() -> {\n"
-   "int b = 32;\n"
-   "return 3;\n"
-   "  },\n"
+   "  []() -> auto {\n"
+   "int b = 32;\n"
+   "return 3;\n"
+   "  },\n"
"  foo, bar)\n"
"  .foo();\n"
"}",
Style);
-  verifyFormat("test() {\n"
-   "  ([]() -> {\n"
+  verifyFormat("void test() {\n"
+   "  ([]() -> auto {\n"
"int b = 32;\n"
"return 3;\n"
"  })\n"
@@ -21975,54 +21976,82 @@
"  .bar();\n"
"}",
Style);
-  EXPECT_EQ("SomeResult doSomething(SomeObject promise) {\n"
-"  return promise.then(\n"
-"  [this, , someObject = "
-"std::mv(s)](std::vector evaluated) mutable {\n"
-"return 

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:1132
+ State.NextToken->is(TT_LambdaLBrace) && State.Line &&
+ State.Line->Level != 0)) {
+  return State.FirstIndent;

Note that we do not apply "OuterScope" behaviour when the line's indentation 
level is 0 to avoid placing a lambda's closing brace at 0 indentation like this:


```
class Foo :
callback{[] {
}, 
bar,
baz} {}
```

In other words, we generally try to apply this rule at function/block scope but 
not otherwise.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146042/new/

https://reviews.llvm.org/D146042

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

[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Jon Phillips via Phabricator via cfe-commits]

jp4a50 created this revision.
Herald added a subscriber: mgrang.
Herald added a project: All.
jp4a50 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The previous implementation of the option involved a hack which
corrupted the parenthesis state stack.

Specifically, this change fixes github issue #55708, #53212, #52846 and #59954.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146042

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -21914,60 +21914,61 @@
LLVMWithBeforeLambdaBody);
 
   // Lambdas with different indentation styles.
-  Style = getLLVMStyleWithColumns(100);
-  EXPECT_EQ("SomeResult doSomething(SomeObject promise) {\n"
-"  return promise.then(\n"
-"  [this, , someObject = "
-"std::mv(s)](std::vector evaluated) mutable {\n"
-"return someObject.startAsyncAction().then(\n"
-"[this, ](AsyncActionResult result) "
-"mutable { result.processMore(); });\n"
-"  });\n"
-"}\n",
-format("SomeResult doSomething(SomeObject promise) {\n"
-   "  return promise.then([this, , someObject = "
-   "std::mv(s)](std::vector evaluated) mutable {\n"
-   "return someObject.startAsyncAction().then([this, "
-   "](AsyncActionResult result) mutable {\n"
-   "  result.processMore();\n"
-   "});\n"
-   "  });\n"
-   "}\n",
-   Style));
+  Style = getLLVMStyleWithColumns(60);
+  verifyFormat("Result doSomething(Promise promise) {\n"
+   "  return promise.then(\n"
+   "  [this, obj = std::move(s)](int evaluated) mutable {\n"
+   "return someObject.startAsyncAction().then(\n"
+   "[this, ](Result result) mutable {\n"
+   "  result.processMore();\n"
+   "});\n"
+   "  });\n"
+   "}\n",
+   Style);
   Style.LambdaBodyIndentation = FormatStyle::LBI_OuterScope;
-  verifyFormat("test() {\n"
-   "  ([]() -> {\n"
+  verifyFormat("Result doSomething(Promise promise) {\n"
+   "  return promise.then(\n"
+   "  [this, obj = std::move(s)](int bar) mutable {\n"
+   "return obj.startAsyncAction().then(\n"
+   "[this, ](Result result) mutable {\n"
+   "  result.processMore();\n"
+   "});\n"
+   "  });\n"
+   "}\n",
+   Style);
+  verifyFormat("void test() {\n"
+   "  ([]() -> auto {\n"
"int b = 32;\n"
"return 3;\n"
"  }).foo();\n"
"}",
Style);
-  verifyFormat("test() {\n"
-   "  []() -> {\n"
+  verifyFormat("void test() {\n"
+   "  []() -> auto {\n"
"int b = 32;\n"
"return 3;\n"
"  }\n"
"}",
Style);
-  verifyFormat("std::sort(v.begin(), v.end(),\n"
-   "  [](const auto , const auto "
-   ") {\n"
-   "  return someLongArgumentName.someMemberVariable < "
-   "someOtherLongArgumentName.someMemberVariable;\n"
-   "});",
+  verifyFormat("void test() {\n"
+   "  std::sort(v.begin(), v.end(),\n"
+   "[](const auto , const auto ) {\n"
+   "return foo.baz < bar.baz;\n"
+   "  });\n"
+   "}\n",
Style);
-  verifyFormat("test() {\n"
+  verifyFormat("void test() {\n"
"  (\n"
-   "  []() -> {\n"
-   "int b = 32;\n"
-   "return 3;\n"
-   "  },\n"
+   "  []() -> auto {\n"
+   "int b = 32;\n"
+   "return 3;\n"
+   "  },\n"
"  foo, bar)\n"
"  .foo();\n"
"}",
Style);
-  verifyFormat("test() {\n"
-   "  ([]() -> {\n"
+  verifyFormat("void test() {\n"
+   "  ([]() -> auto {\n"
"int b = 32;\n"
"return 3;\n"
"  })\n"
@@ -21975,54 +21976,82 @@
"  .bar();\n"
"}",
Style);
-  EXPECT_EQ("SomeResult doSomething(SomeObject promise) {\n"
-"  return promise.then(\n"
-"