[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

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

Add the definition, documentation, and implementation for a new clang-format 
option.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125171

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp

Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -4042,6 +4042,18 @@
   return true;
   }
 
+  // If AlwaysBreakBeforeFunctionParameters is true, we want to break before
+  // the next parameter, if there is one.
+  if (Left.is(tok::l_paren) && Style.AlwaysBreakBeforeFunctionParameters &&
+  !Right.is(tok::r_paren)) {
+if (Left.Previous) {
+  const FormatToken &TwoPrevious = *Left.Previous;
+  if (TwoPrevious.is(TT_FunctionDeclarationName)) {
+return true;
+  }
+}
+  }
+
   // If the last token before a '}', ']', or ')' is a comma or a trailing
   // comment, the intention is to insert a line break after it in order to make
   // shuffling around entries easier. Import statements, especially in
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -685,6 +685,8 @@
 
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
Style.AlwaysBreakBeforeMultilineStrings);
+IO.mapOptional("AlwaysBreakBeforeFunctionParameters",
+   Style.AlwaysBreakBeforeFunctionParameters);
 IO.mapOptional("AlwaysBreakTemplateDeclarations",
Style.AlwaysBreakTemplateDeclarations);
 IO.mapOptional("AttributeMacros", Style.AttributeMacros);
@@ -1367,6 +1369,7 @@
   FormatStyle::SIS_WithoutElse;
   GoogleStyle.AllowShortLoopsOnASingleLine = true;
   GoogleStyle.AlwaysBreakBeforeMultilineStrings = true;
+  GoogleStyle.AlwaysBreakBeforeFunctionParameters = false;
   GoogleStyle.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_Yes;
   GoogleStyle.DerivePointerAlignment = true;
   GoogleStyle.IncludeStyle.IncludeCategories = {{"^", 2, 0, false},
@@ -1438,6 +1441,7 @@
 GoogleStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty;
 GoogleStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
 GoogleStyle.AlwaysBreakBeforeMultilineStrings = false;
+GoogleStyle.AlwaysBreakBeforeFunctionParameters = false;
 GoogleStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
 GoogleStyle.ColumnLimit = 100;
 GoogleStyle.SpaceAfterCStyleCast = true;
@@ -1449,6 +1453,7 @@
 // TODO: still under discussion whether to switch to SLS_All.
 GoogleStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_Empty;
 GoogleStyle.AlwaysBreakBeforeMultilineStrings = false;
+GoogleStyle.AlwaysBreakBeforeFunctionParameters = false;
 GoogleStyle.BreakBeforeTernaryOperators = false;
 // taze:, triple slash directives (`/// <...`), tslint:, and @see, which is
 // commonly followed by overlong URLs.
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -335,6 +335,11 @@
 auto LambdaBodyLength = getLengthToMatchingParen(Current, State.Stack);
 return LambdaBodyLength > getColumnLimit(State);
   }
+  // Check if we want to break before function parameters in declarations
+  if (startsNextParameter(Current, Style) &&
+  Style.AlwaysBreakBeforeFunctionParameters &&
+  State.Line->MustBeDeclaration)
+return true;
   if (Current.MustBreakBefore || Current.is(TT_InlineASMColon))
 return true;
   if (CurrentState.BreakBeforeClosingBrace &&
@@ -983,7 +988,9 @@
 // If we are breaking after '(', '{', '<', or this is the break after a ':'
 // to start a member initializater list in a constructor, this should not
 // be considered bin packing unless the relevant AllowAll option is false or
-// this is a dict/object literal.
+// this is a dict/object literal. Break if
+// AlwaysBreakBeforeFunctionParameters is true and it's a function
+// declaration.
 bool PreviousIsBreakingCtorInitializerColon =
 Previous.is(TT_CtorInitializerColon) &&
 Style.BreakConstructorInitializers == FormatStyle::BCIS_AfterColon;
@@ -995,7 +1002,9 @@
  !State.Line->MustBeDeclaration) ||
 (Style.PackConstructorInitializers != FormatStyle::PCIS_NextLine &&
  PreviousIsBreakingCtorInitializerColon) ||
-Previous.is(TT_DictLiteral))
+Previous.is(TT_DictLiteral) ||
+(Styl

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin added a comment.

In D125171#4167866 , @owenpan wrote:

> Please see 
> https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options.

I am doing this for my team, which writes the security endpoint for Elastic 
Defend. The code is currently private, though the binaries are free to download 
and run. The number of contributors is around 30, and the lines of code is in 
the 100Ks (around 500K). I have not found a way to accomplish what this does 
with the available options. I am adding tests and am happy to maintain this. It 
is a rather small addition, so it is quite simple to keep updated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

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

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin marked 4 inline comments as done.
jrmolin added inline comments.



Comment at: clang/include/clang/Format/Format.h:827
+  /// \code
+  ///   someFunction(
+  ///   int argument1,

HazardyKnusperkeks wrote:
> That's not a valid declaration (missing return type), so not a good example.
fixed. creating a new patch with all the requested context.



Comment at: clang/include/clang/Format/Format.h:831
+  /// \endcode
+  bool AlwaysBreakBeforeFunctionParameters;
+

HazardyKnusperkeks wrote:
> HazardyKnusperkeks wrote:
> > Please sort alphabetically. (So above `AlwaysBreakBeforeMultilineStrings`.)
> You are missing the \since 17.
`\version 17`? I added this locally and am generating a new patch, with all the 
requested context.



Comment at: clang/include/clang/Format/Format.h:831
+  /// \endcode
+  bool AlwaysBreakBeforeFunctionParameters;
+

jrmolin wrote:
> HazardyKnusperkeks wrote:
> > HazardyKnusperkeks wrote:
> > > Please sort alphabetically. (So above 
> > > `AlwaysBreakBeforeMultilineStrings`.)
> > You are missing the \since 17.
> `\version 17`? I added this locally and am generating a new patch, with all 
> the requested context.
sorted. creating a patch with requested context



Comment at: clang/lib/Format/TokenAnnotator.cpp:4740-4742
+  if (Left.is(tok::l_paren) && Style.AlwaysBreakBeforeFunctionParameters &&
+  !Right.is(tok::r_paren)) {
+if (Left.Previous) {

HazardyKnusperkeks wrote:
> Merge the `if`s.
I'm not sure I collapsed what you wanted. I am generating a new patch with all 
the context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

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

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin updated this revision to Diff 505215.
jrmolin marked 3 inline comments as done.
jrmolin added a comment.

respond to code review requests/comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.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
@@ -25346,6 +25346,27 @@
   verifyFormat("auto x = 5s .count() == 5;");
 }
 
+TEST_F(FormatTest, BreakBeforeParameterList) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.AlwaysBreakBeforeFunctionParameters, false);
+
+  const StringRef Code("int function1(int param1, int param2, int param3);\n"
+   "int function2();\n");
+
+  // verify that there is no break by default
+  verifyFormat("int function1(int param1, int param2, int param3);\n"
+   "int function2();\n",
+   Code, Style);
+
+  // verify that there is a break when told to break
+  Style.AlwaysBreakBeforeFunctionParameters = true;
+  verifyFormat("int function1(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function2();\n",
+   Code, Style);
+}
 } // namespace
 } // namespace test
 } // namespace format
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -4737,6 +4737,16 @@
 return true;
   }
 
+  // If AlwaysBreakBeforeFunctionParameters is true, we want to break before
+  // the next parameter, if there is one.
+  if (Left.is(tok::l_paren) && Style.AlwaysBreakBeforeFunctionParameters &&
+  !Right.is(tok::r_paren) && Left.Previous) {
+  const FormatToken &TwoPrevious = *Left.Previous;
+  if (TwoPrevious.is(TT_FunctionDeclarationName)) {
+  return true;
+  }
+  }
+
   // If the last token before a '}', ']', or ')' is a comma or a trailing
   // comment, the intention is to insert a line break after it in order to make
   // shuffling around entries easier. Import statements, especially in
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -870,6 +870,8 @@
Style.AlwaysBreakAfterReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
Style.AlwaysBreakBeforeMultilineStrings);
+IO.mapOptional("AlwaysBreakBeforeFunctionParameters",
+   Style.AlwaysBreakBeforeFunctionParameters);
 IO.mapOptional("AlwaysBreakTemplateDeclarations",
Style.AlwaysBreakTemplateDeclarations);
 IO.mapOptional("AttributeMacros", Style.AttributeMacros);
@@ -1325,6 +1327,7 @@
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
+  LLVMStyle.AlwaysBreakBeforeFunctionParameters = false;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_MultiLine;
   LLVMStyle.AttributeMacros.push_back("__capability");
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -354,6 +354,11 @@
 auto LambdaBodyLength = getLengthToMatchingParen(Current, State.Stack);
 return LambdaBodyLength > getColumnLimit(State);
   }
+  // Check if we want to break before function parameters in declarations
+  if (startsNextParameter(Current, Style) &&
+  Style.AlwaysBreakBeforeFunctionParameters &&
+  State.Line->MustBeDeclaration)
+return true;
   if (Current.MustBreakBefore ||
   (Current.is(TT_InlineASMColon) &&
(Style.BreakBeforeInlineASMColon == FormatStyle::BBIAS_Always ||
@@ -1055,7 +1060,9 @@
 // If we are breaking after '(', '{', '<', or this is the break after a ':'
 // to start a member initializater list in a constructor, this should not
 // be considered bin packing unless the relevant AllowAll option is false or
-// this is a dict/object literal.
+// this is a dict/object literal. Break if
+// AlwaysBreakBeforeFunctionParameters is true and it's a function
+// declaration.
 bool PreviousIsBreakingCtorInitializerColon =
 PreviousNonComment && PreviousNonComment->is(TT_CtorInitializerColon) &&
 Style.BreakConst

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin added a comment.

In D125171#4195298 , @owenpan wrote:

> In D125171#4193996 , @jrmolin wrote:
>
>> In D125171#4167866 , @owenpan 
>> wrote:
>>
>>> Please see 
>>> https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options.
>>
>> I am doing this for my team, which writes the security endpoint for Elastic 
>> Defend. The code is currently private, though the binaries are free to 
>> download and run. The number of contributors is around 30, and the lines of 
>> code is in the 100Ks (around 500K). I have not found a way to accomplish 
>> what this does with the available options. I am adding tests and am happy to 
>> maintain this. It is a rather small addition, so it is quite simple to keep 
>> updated.
>
> Do you "have a publicly accessible style guide"?

Unfortunately, we don't. I can create one in the likeness of the others, but 
I'm not sure where it will get published. I will talk to my managers about 
publishing a style doc.




Comment at: clang/docs/ClangFormatStyleOptions.rst:1372
+.. _AlwaysBreakBeforeFunctionParameters:
+
+**AlwaysBreakBeforeFunctionParameters** (``Boolean``) 
:versionbadge:`clang-format 16.0`

MyDeveloperDay wrote:
> Regererate
How do I re-generate? I'm happy to! I just don't see where to do that.



Comment at: clang/lib/Format/Format.cpp:872
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
Style.AlwaysBreakBeforeMultilineStrings);
+IO.mapOptional("AlwaysBreakBeforeFunctionParameters",

MyDeveloperDay wrote:
> Needs a parse test
I will add one. Thanks for the reminder!



Comment at: clang/unittests/Format/FormatTest.cpp:25357
+  // verify that there is no break by default
+  verifyFormat("int function1(int param1, int param2, int param3);\n"
+   "int function2();\n",

MyDeveloperDay wrote:
> Please write it out long form
What do you mean by "long form"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

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

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin updated this revision to Diff 507539.
jrmolin added a comment.

I updated the code comment in Format.h and ran the docs generator. I didn't 
modify the Macros section, but that got updated when I ran the docs generator.

We don't have a published style guide, unfortunately. I can work towards that, 
but I don't know how long that would take. My team doesn't like to agree on 
formatting changes. Hence this patch. We rolled this into the 3.9.0 release, 
and have been stuck with that ever since.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.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
@@ -25350,6 +25350,27 @@
   verifyFormat("auto x = 5s .count() == 5;");
 }
 
+TEST_F(FormatTest, BreakBeforeParameterList) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.AlwaysBreakBeforeFunctionParameters, false);
+
+  const StringRef Code("int function1(int param1, int param2, int param3);\n"
+   "int function2();\n");
+
+  // verify that there is no break by default
+  verifyFormat("int function1(int param1, int param2, int param3);\n"
+   "int function2();\n",
+   Code, Style);
+
+  // verify that there is a break when told to break
+  Style.AlwaysBreakBeforeFunctionParameters = true;
+  verifyFormat("int function1(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function2();\n",
+   Code, Style);
+}
 } // namespace
 } // namespace test
 } // namespace format
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -4737,6 +4737,16 @@
 return true;
   }
 
+  // If AlwaysBreakBeforeFunctionParameters is true, we want to break before
+  // the next parameter, if there is one.
+  if (Left.is(tok::l_paren) && Style.AlwaysBreakBeforeFunctionParameters &&
+  !Right.is(tok::r_paren) && Left.Previous) {
+  const FormatToken &TwoPrevious = *Left.Previous;
+  if (TwoPrevious.is(TT_FunctionDeclarationName)) {
+  return true;
+  }
+  }
+
   // If the last token before a '}', ']', or ')' is a comma or a trailing
   // comment, the intention is to insert a line break after it in order to make
   // shuffling around entries easier. Import statements, especially in
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -870,6 +870,8 @@
Style.AlwaysBreakAfterReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
Style.AlwaysBreakBeforeMultilineStrings);
+IO.mapOptional("AlwaysBreakBeforeFunctionParameters",
+   Style.AlwaysBreakBeforeFunctionParameters);
 IO.mapOptional("AlwaysBreakTemplateDeclarations",
Style.AlwaysBreakTemplateDeclarations);
 IO.mapOptional("AttributeMacros", Style.AttributeMacros);
@@ -1325,6 +1327,7 @@
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
+  LLVMStyle.AlwaysBreakBeforeFunctionParameters = false;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_MultiLine;
   LLVMStyle.AttributeMacros.push_back("__capability");
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -354,6 +354,11 @@
 auto LambdaBodyLength = getLengthToMatchingParen(Current, State.Stack);
 return LambdaBodyLength > getColumnLimit(State);
   }
+  // Check if we want to break before function parameters in declarations
+  if (startsNextParameter(Current, Style) &&
+  Style.AlwaysBreakBeforeFunctionParameters &&
+  State.Line->MustBeDeclaration)
+return true;
   if (Current.MustBreakBefore ||
   (Current.is(TT_InlineASMColon) &&
(Style.BreakBeforeInlineASMColon == FormatStyle::BBIAS_Always ||
@@ -1055,7 +1060,9 @@
 // If we are breaking after '(', '{', '<', or this is the break after a ':'
 // to start a member initializater list in a constructor, this should not
 // be considered bin packing unless the relevant AllowAll option

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

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

I still don't know what `Please write it out long form` means for the unit test.




Comment at: clang/docs/ClangFormatStyleOptions.rst:3663
 
+.. _Macros:
+

I didn't modify this section in the header at all. I assume someone modified 
the Format.h file and didn't update the docs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

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

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin added a comment.

In D125171#4215910 , @MyDeveloperDay 
wrote:

> if you go ahead an rebase your should get rG7a5b95732ade: [clang-format] NFC 
> Format.h and ClangFormatStyleOptions.rst are out of date 
>  that 
> will remove the need for the Macro section in the rst

So pull in that git hash and re-diff? I couldn't get arcanist installed cleanly 
(php issues), so I've been using `git diff -U99...` to generate the 
patches. I just keep pulling down main before creating a patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

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

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin updated this revision to Diff 508639.
jrmolin marked 2 inline comments as done.
jrmolin added a comment.

pulled main, re-generated the docs, then re-generated the diff. Also added a 
parse test and collapsed some nested `if` conditions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.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
@@ -25388,6 +25388,27 @@
   verifyFormat("auto x = 5s .count() == 5;");
 }
 
+TEST_F(FormatTest, BreakBeforeParameterList) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.AlwaysBreakBeforeFunctionParameters, false);
+
+  const StringRef Code("int function1(int param1, int param2, int param3);\n"
+   "int function2();\n");
+
+  // verify that there is no break by default
+  verifyFormat("int function1(int param1, int param2, int param3);\n"
+   "int function2();\n",
+   Code, Style);
+
+  // verify that there is a break when told to break
+  Style.AlwaysBreakBeforeFunctionParameters = true;
+  verifyFormat("int function1(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function2();\n",
+   Code, Style);
+}
 } // namespace
 } // namespace test
 } // namespace format
Index: clang/unittests/Format/ConfigParseTest.cpp
===
--- clang/unittests/Format/ConfigParseTest.cpp
+++ clang/unittests/Format/ConfigParseTest.cpp
@@ -149,6 +149,7 @@
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortEnumsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
+  CHECK_PARSE_BOOL(AlwaysBreakBeforeFunctionParameters);
   CHECK_PARSE_BOOL(BinPackArguments);
   CHECK_PARSE_BOOL(BinPackParameters);
   CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -4782,6 +4782,13 @@
 return true;
   }
 
+  // If AlwaysBreakBeforeFunctionParameters is true, we want to break before
+  // the next parameter, if there is one.
+  if (Left.is(tok::l_paren) && Style.AlwaysBreakBeforeFunctionParameters &&
+  !Right.is(tok::r_paren) && Left.Previous && Left.Previous->is(TT_FunctionDeclarationName)) {
+  return true;
+  }
+
   // If the last token before a '}', ']', or ')' is a comma or a trailing
   // comment, the intention is to insert a line break after it in order to make
   // shuffling around entries easier. Import statements, especially in
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -870,6 +870,8 @@
Style.AlwaysBreakAfterReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
Style.AlwaysBreakBeforeMultilineStrings);
+IO.mapOptional("AlwaysBreakBeforeFunctionParameters",
+   Style.AlwaysBreakBeforeFunctionParameters);
 IO.mapOptional("AlwaysBreakTemplateDeclarations",
Style.AlwaysBreakTemplateDeclarations);
 IO.mapOptional("AttributeMacros", Style.AttributeMacros);
@@ -1325,6 +1327,7 @@
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
+  LLVMStyle.AlwaysBreakBeforeFunctionParameters = false;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_MultiLine;
   LLVMStyle.AttributeMacros.push_back("__capability");
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -354,6 +354,11 @@
 auto LambdaBodyLength = getLengthToMatchingParen(Current, State.Stack);
 return LambdaBodyLength > getColumnLimit(State);
   }
+  // Check if we want to break before function parameters in declarations
+  if (startsNextParameter(Current, Style) &&
+  Style.AlwaysBreakBeforeFunctionParameters &&
+  State.Line->MustBeDeclaration)
+return true;
   if (Current.MustBreakBefore ||
   (Current.is(TT_InlineASMColon) &&
(Style.BreakBeforeInline

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin marked an inline comment as done.
jrmolin added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:4742-4748
+  if (Left.is(tok::l_paren) && Style.AlwaysBreakBeforeFunctionParameters &&
+  !Right.is(tok::r_paren) && Left.Previous) {
+  const FormatToken &TwoPrevious = *Left.Previous;
+  if (TwoPrevious.is(TT_FunctionDeclarationName)) {
+  return true;
+  }
+  }

HazardyKnusperkeks wrote:
> That I meant with merge.
Ah, understood. I generally don't collapse `if`s, so I didn't know how much you 
wanted. It seems like ... all of it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

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

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin updated this revision to Diff 509317.
jrmolin added a comment.

ran the formatter, ran the documentation generator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.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
@@ -25393,6 +25393,27 @@
   verifyFormat("auto x = 5s .count() == 5;");
 }
 
+TEST_F(FormatTest, BreakBeforeParameterList) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.AlwaysBreakBeforeFunctionParameters, false);
+
+  const StringRef Code("int function1(int param1, int param2, int param3);\n"
+   "int function2();\n");
+
+  // verify that there is no break by default
+  verifyFormat("int function1(int param1, int param2, int param3);\n"
+   "int function2();\n",
+   Code, Style);
+
+  // verify that there is a break when told to break
+  Style.AlwaysBreakBeforeFunctionParameters = true;
+  verifyFormat("int function1(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function2();\n",
+   Code, Style);
+}
 } // namespace
 } // namespace test
 } // namespace format
Index: clang/unittests/Format/ConfigParseTest.cpp
===
--- clang/unittests/Format/ConfigParseTest.cpp
+++ clang/unittests/Format/ConfigParseTest.cpp
@@ -149,6 +149,7 @@
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortEnumsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
+  CHECK_PARSE_BOOL(AlwaysBreakBeforeFunctionParameters);
   CHECK_PARSE_BOOL(BinPackArguments);
   CHECK_PARSE_BOOL(BinPackParameters);
   CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -4783,6 +4783,14 @@
 return true;
   }
 
+  // If AlwaysBreakBeforeFunctionParameters is true, we want to break before
+  // the next parameter, if there is one.
+  if (Left.is(tok::l_paren) && Style.AlwaysBreakBeforeFunctionParameters &&
+  !Right.is(tok::r_paren) && Left.Previous &&
+  Left.Previous->is(TT_FunctionDeclarationName)) {
+return true;
+  }
+
   // If the last token before a '}', ']', or ')' is a comma or a trailing
   // comment, the intention is to insert a line break after it in order to make
   // shuffling around entries easier. Import statements, especially in
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -870,6 +870,8 @@
Style.AlwaysBreakAfterReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
Style.AlwaysBreakBeforeMultilineStrings);
+IO.mapOptional("AlwaysBreakBeforeFunctionParameters",
+   Style.AlwaysBreakBeforeFunctionParameters);
 IO.mapOptional("AlwaysBreakTemplateDeclarations",
Style.AlwaysBreakTemplateDeclarations);
 IO.mapOptional("AttributeMacros", Style.AttributeMacros);
@@ -1325,6 +1327,7 @@
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
+  LLVMStyle.AlwaysBreakBeforeFunctionParameters = false;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_MultiLine;
   LLVMStyle.AttributeMacros.push_back("__capability");
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -354,6 +354,12 @@
 auto LambdaBodyLength = getLengthToMatchingParen(Current, State.Stack);
 return LambdaBodyLength > getColumnLimit(State);
   }
+  // Check if we want to break before function parameters in declarations
+  if (startsNextParameter(Current, Style) &&
+  Style.AlwaysBreakBeforeFunctionParameters &&
+  State.Line->MustBeDeclaration) {
+return true;
+  }
   if (Current.MustBreakBefore ||
   (Current.is(TT_InlineASMColon) &&
(Style.BreakBeforeInlineASMColon == FormatStyle::BBIAS_Always ||
@@ -1055,7 +1061,9 @@
 // If we are breaking after '(', '{', '<', or 

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin added a comment.

I think I have hit all the requests now. We're in the middle of building 
release candidates and testing, so management is taking longer and longer to 
get back to me about a style guide for my team.

We added it, because it forces a consistent look across all function 
declarations. I don't know what the next steps are now. I'm stuck; you're 
stuck. :shrug:




Comment at: clang/lib/Format/TokenAnnotator.cpp:4742-4748
+  if (Left.is(tok::l_paren) && Style.AlwaysBreakBeforeFunctionParameters &&
+  !Right.is(tok::r_paren) && Left.Previous) {
+  const FormatToken &TwoPrevious = *Left.Previous;
+  if (TwoPrevious.is(TT_FunctionDeclarationName)) {
+  return true;
+  }
+  }

HazardyKnusperkeks wrote:
> jrmolin wrote:
> > HazardyKnusperkeks wrote:
> > > That I meant with merge.
> > Ah, understood. I generally don't collapse `if`s, so I didn't know how much 
> > you wanted. It seems like ... all of it.
> I think now you need to format it.
I ran the formatting


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

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

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin updated this revision to Diff 501836.
jrmolin added a comment.

Updated the version string in the docs and pulled in main.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp

Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -4724,6 +4724,18 @@
 return true;
   }
 
+  // If AlwaysBreakBeforeFunctionParameters is true, we want to break before
+  // the next parameter, if there is one.
+  if (Left.is(tok::l_paren) && Style.AlwaysBreakBeforeFunctionParameters &&
+  !Right.is(tok::r_paren)) {
+if (Left.Previous) {
+  const FormatToken &TwoPrevious = *Left.Previous;
+  if (TwoPrevious.is(TT_FunctionDeclarationName)) {
+return true;
+  }
+}
+  }
+
   // If the last token before a '}', ']', or ')' is a comma or a trailing
   // comment, the intention is to insert a line break after it in order to make
   // shuffling around entries easier. Import statements, especially in
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -870,6 +870,8 @@
Style.AlwaysBreakAfterReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
Style.AlwaysBreakBeforeMultilineStrings);
+IO.mapOptional("AlwaysBreakBeforeFunctionParameters",
+   Style.AlwaysBreakBeforeFunctionParameters);
 IO.mapOptional("AlwaysBreakTemplateDeclarations",
Style.AlwaysBreakTemplateDeclarations);
 IO.mapOptional("AttributeMacros", Style.AttributeMacros);
@@ -1504,6 +1506,7 @@
   FormatStyle::SIS_WithoutElse;
   GoogleStyle.AllowShortLoopsOnASingleLine = true;
   GoogleStyle.AlwaysBreakBeforeMultilineStrings = true;
+  GoogleStyle.AlwaysBreakBeforeFunctionParameters = false;
   GoogleStyle.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_Yes;
   GoogleStyle.DerivePointerAlignment = true;
   GoogleStyle.IncludeStyle.IncludeCategories = {{"^", 2, 0, false},
@@ -1576,6 +1579,7 @@
 GoogleStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty;
 GoogleStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
 GoogleStyle.AlwaysBreakBeforeMultilineStrings = false;
+GoogleStyle.AlwaysBreakBeforeFunctionParameters = false;
 GoogleStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
 GoogleStyle.ColumnLimit = 100;
 GoogleStyle.SpaceAfterCStyleCast = true;
@@ -1587,6 +1591,7 @@
 // TODO: still under discussion whether to switch to SLS_All.
 GoogleStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_Empty;
 GoogleStyle.AlwaysBreakBeforeMultilineStrings = false;
+GoogleStyle.AlwaysBreakBeforeFunctionParameters = false;
 GoogleStyle.BreakBeforeTernaryOperators = false;
 // taze:, triple slash directives (`/// <...`), tslint:, and @see, which is
 // commonly followed by overlong URLs.
@@ -1612,6 +1617,7 @@
 GoogleStyle.BreakStringLiterals = false;
   } else if (Language == FormatStyle::LK_ObjC) {
 GoogleStyle.AlwaysBreakBeforeMultilineStrings = false;
+GoogleStyle.AlwaysBreakBeforeFunctionParameters = false;
 GoogleStyle.ColumnLimit = 100;
 // "Regroup" doesn't work well for ObjC yet (main header heuristic,
 // relationship between ObjC standard library headers and other heades,
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -353,6 +353,11 @@
 auto LambdaBodyLength = getLengthToMatchingParen(Current, State.Stack);
 return LambdaBodyLength > getColumnLimit(State);
   }
+  // Check if we want to break before function parameters in declarations
+  if (startsNextParameter(Current, Style) &&
+  Style.AlwaysBreakBeforeFunctionParameters &&
+  State.Line->MustBeDeclaration)
+return true;
   if (Current.MustBreakBefore ||
   (Current.is(TT_InlineASMColon) &&
(Style.BreakBeforeInlineASMColon == FormatStyle::BBIAS_Always ||
@@ -1049,7 +1054,9 @@
 // If we are breaking after '(', '{', '<', or this is the break after a ':'
 // to start a member initializater list in a constructor, this should not
 // be considered bin packing unless the relevant AllowAll option is false or
-// this is a dict/object literal.
+// this is a dict/object literal. Break if
+// AlwaysBreakBeforeFunctionParameters is true and it's a function
+// declaration.
 bool PreviousIsBreakingCto

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin added reviewers: MyDeveloperDay, owenpan.
jrmolin added a comment.

I finally found the correct CodeOwners.rst file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

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

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin updated this revision to Diff 502112.
jrmolin added a comment.

Finally figured out how to run the latest `git-clang-format` on the code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp

Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -4735,6 +4735,17 @@
 return true;
   }
 
+  // If AlwaysBreakBeforeFunctionParameters is true, we want to break before
+  // the next parameter, if there is one.
+  if (Left.is(tok::l_paren) && Style.AlwaysBreakBeforeFunctionParameters &&
+  !Right.is(tok::r_paren)) {
+if (Left.Previous) {
+  const FormatToken &TwoPrevious = *Left.Previous;
+  if (TwoPrevious.is(TT_FunctionDeclarationName))
+return true;
+}
+  }
+
   // If the last token before a '}', ']', or ')' is a comma or a trailing
   // comment, the intention is to insert a line break after it in order to make
   // shuffling around entries easier. Import statements, especially in
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -870,6 +870,8 @@
Style.AlwaysBreakAfterReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
Style.AlwaysBreakBeforeMultilineStrings);
+IO.mapOptional("AlwaysBreakBeforeFunctionParameters",
+   Style.AlwaysBreakBeforeFunctionParameters);
 IO.mapOptional("AlwaysBreakTemplateDeclarations",
Style.AlwaysBreakTemplateDeclarations);
 IO.mapOptional("AttributeMacros", Style.AttributeMacros);
@@ -1505,6 +1507,7 @@
   FormatStyle::SIS_WithoutElse;
   GoogleStyle.AllowShortLoopsOnASingleLine = true;
   GoogleStyle.AlwaysBreakBeforeMultilineStrings = true;
+  GoogleStyle.AlwaysBreakBeforeFunctionParameters = false;
   GoogleStyle.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_Yes;
   GoogleStyle.DerivePointerAlignment = true;
   GoogleStyle.IncludeStyle.IncludeCategories = {{"^", 2, 0, false},
@@ -1577,6 +1580,7 @@
 GoogleStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty;
 GoogleStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
 GoogleStyle.AlwaysBreakBeforeMultilineStrings = false;
+GoogleStyle.AlwaysBreakBeforeFunctionParameters = false;
 GoogleStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
 GoogleStyle.ColumnLimit = 100;
 GoogleStyle.SpaceAfterCStyleCast = true;
@@ -1588,6 +1592,7 @@
 // TODO: still under discussion whether to switch to SLS_All.
 GoogleStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_Empty;
 GoogleStyle.AlwaysBreakBeforeMultilineStrings = false;
+GoogleStyle.AlwaysBreakBeforeFunctionParameters = false;
 GoogleStyle.BreakBeforeTernaryOperators = false;
 // taze:, triple slash directives (`/// <...`), tslint:, and @see, which is
 // commonly followed by overlong URLs.
@@ -1613,6 +1618,7 @@
 GoogleStyle.BreakStringLiterals = false;
   } else if (Language == FormatStyle::LK_ObjC) {
 GoogleStyle.AlwaysBreakBeforeMultilineStrings = false;
+GoogleStyle.AlwaysBreakBeforeFunctionParameters = false;
 GoogleStyle.ColumnLimit = 100;
 // "Regroup" doesn't work well for ObjC yet (main header heuristic,
 // relationship between ObjC standard library headers and other heades,
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -354,6 +354,12 @@
 auto LambdaBodyLength = getLengthToMatchingParen(Current, State.Stack);
 return LambdaBodyLength > getColumnLimit(State);
   }
+  // Check if we want to break before function parameters in declarations
+  if (startsNextParameter(Current, Style) &&
+  Style.AlwaysBreakBeforeFunctionParameters &&
+  State.Line->MustBeDeclaration) {
+return true;
+  }
   if (Current.MustBreakBefore ||
   (Current.is(TT_InlineASMColon) &&
(Style.BreakBeforeInlineASMColon == FormatStyle::BBIAS_Always ||
@@ -1055,7 +1061,9 @@
 // If we are breaking after '(', '{', '<', or this is the break after a ':'
 // to start a member initializater list in a constructor, this should not
 // be considered bin packing unless the relevant AllowAll option is false or
-// this is a dict/object literal.
+// this is a dict/object literal. Break if
+// AlwaysBreakBeforeFunctionParameters is true and it's a function
+// declaration.
 bool PreviousIsB

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin added a comment.

In D125171#4230397 , @jrmolin wrote:

> I think I have hit all the requests now. We're in the middle of building 
> release candidates and testing, so management is taking longer and longer to 
> get back to me about a style guide for my team.
>
> We added it, because it forces a consistent look across all function 
> declarations. I don't know what the next steps are now. I'm stuck; you're 
> stuck. :shrug:

I started a style guide at 
https://github.com/elastic/endpoint/blob/add_style_guide/style_guide.md, but it 
hasn't been approved or merged to `main`. It's at least a start...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

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

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin updated this revision to Diff 512447.
jrmolin marked 3 inline comments as done.
jrmolin added a comment.

- updated the tests to use long-form
- added new tests to verify the no-parameter cases
- updated the documentation to minimize formatting variables
- ran the style guide generator


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.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
@@ -25429,6 +25429,50 @@
   verifyFormat("auto x = 5s .count() == 5;");
 }
 
+TEST_F(FormatTest, BreakBeforeParameterList) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.AlwaysBreakBeforeFunctionParameters, false);
+
+  // verify that there is no break by default
+  verifyFormat("int function1(int param1, int param2, int param3);\n"
+   "int function2();\n",
+   "int function1(int param1, int param2, int param3);\n"
+   "int function2();\n",
+   Style);
+
+  verifyFormat("void function1(int param1, int param2, int param3) {}\n",
+   "void function1(int param1, int param2, int param3) {}\n",
+   Style);
+
+  // verify that there is a break when told to break
+  Style.AlwaysBreakBeforeFunctionParameters = true;
+  verifyFormat("int function1(\n" // the formatted part
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function2();\n",
+   // the original
+   "int function1(int param1, int param2, int param3);\n"
+   "int function2();\n",
+   Style);
+
+  verifyFormat("void function1(\n" // the formatted part
+   "int param1,\n"
+   "int param2,\n"
+   "int param3) {}\n",
+   // the original
+   "void function1(int param1, int param2, int param3) {}\n",
+   Style);
+
+  // verify that having no parameters doesn't affect the parentheses
+  verifyFormat("void function1() {}\n", // the formatted part
+   "void function1() {}\n", // the original
+   Style);
+
+  verifyFormat("void function1();\n", // the formatted part
+   "void function1();\n", // the original
+   Style);
+}
 } // namespace
 } // namespace test
 } // namespace format
Index: clang/unittests/Format/ConfigParseTest.cpp
===
--- clang/unittests/Format/ConfigParseTest.cpp
+++ clang/unittests/Format/ConfigParseTest.cpp
@@ -149,6 +149,7 @@
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortEnumsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
+  CHECK_PARSE_BOOL(AlwaysBreakBeforeFunctionParameters);
   CHECK_PARSE_BOOL(BinPackArguments);
   CHECK_PARSE_BOOL(BinPackParameters);
   CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -4847,6 +4847,14 @@
 return true;
   }
 
+  // If AlwaysBreakBeforeFunctionParameters is true, we want to break before
+  // the next parameter, if there is one.
+  if (Left.is(tok::l_paren) && Style.AlwaysBreakBeforeFunctionParameters &&
+  !Right.is(tok::r_paren) && Left.Previous &&
+  Left.Previous->is(TT_FunctionDeclarationName)) {
+return true;
+  }
+
   // If the last token before a '}', ']', or ')' is a comma or a trailing
   // comment, the intention is to insert a line break after it in order to make
   // shuffling around entries easier. Import statements, especially in
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -873,6 +873,8 @@
Style.AlwaysBreakAfterReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
Style.AlwaysBreakBeforeMultilineStrings);
+IO.mapOptional("AlwaysBreakBeforeFunctionParameters",
+   Style.AlwaysBreakBeforeFunctionParameters);
 IO.mapOptional("AlwaysBreakTemplateDeclarations",
Style.AlwaysBreakTemplateDeclarations);
 IO.mapOptional("AttributeMacros", Style.AttributeMacros);
@@ -1331,6 +1333,7 @@
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLV

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

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



Comment at: clang/docs/ClangFormatStyleOptions.rst:1380
+  Example uses
+  ``AlwaysBreakAfterReturnType`` set to ``All``.
+

MyDeveloperDay wrote:
> This isn't relevant is it? `AlwaysBreakAfterReturnType`
No, it's not relevant, but I saw it in other examples, and I like the way it 
looks. I can change the example to be more minimal.



Comment at: clang/lib/Format/TokenAnnotator.cpp:4789
+  if (Left.is(tok::l_paren) && Style.AlwaysBreakBeforeFunctionParameters &&
+  !Right.is(tok::r_paren) && Left.Previous &&
+  Left.Previous->is(TT_FunctionDeclarationName)) {

MyDeveloperDay wrote:
> can you test the `()` case in your tests please with 
> `AlwaysBreakBeforeFunctionParameters` set to true
updated the tests.



Comment at: clang/unittests/Format/FormatTest.cpp:25410-25415
+  verifyFormat("int function1(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function2();\n",
+   Code, Style);

MyDeveloperDay wrote:
> 
Oh, I see. Sure!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

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

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin updated this revision to Diff 512449.
jrmolin marked an inline comment as done.
jrmolin added a comment.

- updated the documentation to show the option works for function declaration 
and definition


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.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
@@ -25429,6 +25429,50 @@
   verifyFormat("auto x = 5s .count() == 5;");
 }
 
+TEST_F(FormatTest, BreakBeforeParameterList) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.AlwaysBreakBeforeFunctionParameters, false);
+
+  // verify that there is no break by default
+  verifyFormat("int function1(int param1, int param2, int param3);\n"
+   "int function2();\n",
+   "int function1(int param1, int param2, int param3);\n"
+   "int function2();\n",
+   Style);
+
+  verifyFormat("void function1(int param1, int param2, int param3) {}\n",
+   "void function1(int param1, int param2, int param3) {}\n",
+   Style);
+
+  // verify that there is a break when told to break
+  Style.AlwaysBreakBeforeFunctionParameters = true;
+  verifyFormat("int function1(\n" // the formatted part
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function2();\n",
+   // the original
+   "int function1(int param1, int param2, int param3);\n"
+   "int function2();\n",
+   Style);
+
+  verifyFormat("void function1(\n" // the formatted part
+   "int param1,\n"
+   "int param2,\n"
+   "int param3) {}\n",
+   // the original
+   "void function1(int param1, int param2, int param3) {}\n",
+   Style);
+
+  // verify that having no parameters doesn't affect the parentheses
+  verifyFormat("void function1() {}\n", // the formatted part
+   "void function1() {}\n", // the original
+   Style);
+
+  verifyFormat("void function1();\n", // the formatted part
+   "void function1();\n", // the original
+   Style);
+}
 } // namespace
 } // namespace test
 } // namespace format
Index: clang/unittests/Format/ConfigParseTest.cpp
===
--- clang/unittests/Format/ConfigParseTest.cpp
+++ clang/unittests/Format/ConfigParseTest.cpp
@@ -149,6 +149,7 @@
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortEnumsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
+  CHECK_PARSE_BOOL(AlwaysBreakBeforeFunctionParameters);
   CHECK_PARSE_BOOL(BinPackArguments);
   CHECK_PARSE_BOOL(BinPackParameters);
   CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -4847,6 +4847,14 @@
 return true;
   }
 
+  // If AlwaysBreakBeforeFunctionParameters is true, we want to break before
+  // the next parameter, if there is one.
+  if (Left.is(tok::l_paren) && Style.AlwaysBreakBeforeFunctionParameters &&
+  !Right.is(tok::r_paren) && Left.Previous &&
+  Left.Previous->is(TT_FunctionDeclarationName)) {
+return true;
+  }
+
   // If the last token before a '}', ']', or ')' is a comma or a trailing
   // comment, the intention is to insert a line break after it in order to make
   // shuffling around entries easier. Import statements, especially in
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -873,6 +873,8 @@
Style.AlwaysBreakAfterReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
Style.AlwaysBreakBeforeMultilineStrings);
+IO.mapOptional("AlwaysBreakBeforeFunctionParameters",
+   Style.AlwaysBreakBeforeFunctionParameters);
 IO.mapOptional("AlwaysBreakTemplateDeclarations",
Style.AlwaysBreakTemplateDeclarations);
 IO.mapOptional("AttributeMacros", Style.AttributeMacros);
@@ -1331,6 +1333,7 @@
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
+  LLVMStyle.Alw

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin updated this revision to Diff 512480.
jrmolin added a comment.

fix a comment in the documentation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.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
@@ -25429,6 +25429,50 @@
   verifyFormat("auto x = 5s .count() == 5;");
 }
 
+TEST_F(FormatTest, BreakBeforeParameterList) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.AlwaysBreakBeforeFunctionParameters, false);
+
+  // verify that there is no break by default
+  verifyFormat("int function1(int param1, int param2, int param3);\n"
+   "int function2();\n",
+   "int function1(int param1, int param2, int param3);\n"
+   "int function2();\n",
+   Style);
+
+  verifyFormat("void function1(int param1, int param2, int param3) {}\n",
+   "void function1(int param1, int param2, int param3) {}\n",
+   Style);
+
+  // verify that there is a break when told to break
+  Style.AlwaysBreakBeforeFunctionParameters = true;
+  verifyFormat("int function1(\n" // the formatted part
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function2();\n",
+   // the original
+   "int function1(int param1, int param2, int param3);\n"
+   "int function2();\n",
+   Style);
+
+  verifyFormat("void function1(\n" // the formatted part
+   "int param1,\n"
+   "int param2,\n"
+   "int param3) {}\n",
+   // the original
+   "void function1(int param1, int param2, int param3) {}\n",
+   Style);
+
+  // verify that having no parameters doesn't affect the parentheses
+  verifyFormat("void function1() {}\n", // the formatted part
+   "void function1() {}\n", // the original
+   Style);
+
+  verifyFormat("void function1();\n", // the formatted part
+   "void function1();\n", // the original
+   Style);
+}
 } // namespace
 } // namespace test
 } // namespace format
Index: clang/unittests/Format/ConfigParseTest.cpp
===
--- clang/unittests/Format/ConfigParseTest.cpp
+++ clang/unittests/Format/ConfigParseTest.cpp
@@ -149,6 +149,7 @@
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortEnumsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
+  CHECK_PARSE_BOOL(AlwaysBreakBeforeFunctionParameters);
   CHECK_PARSE_BOOL(BinPackArguments);
   CHECK_PARSE_BOOL(BinPackParameters);
   CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -4847,6 +4847,14 @@
 return true;
   }
 
+  // If AlwaysBreakBeforeFunctionParameters is true, we want to break before
+  // the next parameter, if there is one.
+  if (Left.is(tok::l_paren) && Style.AlwaysBreakBeforeFunctionParameters &&
+  !Right.is(tok::r_paren) && Left.Previous &&
+  Left.Previous->is(TT_FunctionDeclarationName)) {
+return true;
+  }
+
   // If the last token before a '}', ']', or ')' is a comma or a trailing
   // comment, the intention is to insert a line break after it in order to make
   // shuffling around entries easier. Import statements, especially in
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -873,6 +873,8 @@
Style.AlwaysBreakAfterReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
Style.AlwaysBreakBeforeMultilineStrings);
+IO.mapOptional("AlwaysBreakBeforeFunctionParameters",
+   Style.AlwaysBreakBeforeFunctionParameters);
 IO.mapOptional("AlwaysBreakTemplateDeclarations",
Style.AlwaysBreakTemplateDeclarations);
 IO.mapOptional("AttributeMacros", Style.AttributeMacros);
@@ -1331,6 +1333,7 @@
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
+  LLVMStyle.AlwaysBreakBeforeFunctionParameters = false;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   

[PATCH] D125171: [clang-format] Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin added a comment.

In D125171#4261301 , @MyDeveloperDay 
wrote:

> is it possible to point to a github issue that this related to in the review 
> summary, if not please can you make one so we can track the issue its trying 
> to solve

added an issue: https://github.com/llvm/llvm-project/issues/62092


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

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

[PATCH] D125171: [clang-format] Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin updated this revision to Diff 513610.
jrmolin added a comment.

changing the added option from a boolean to an enum that takes `Leave`, 
`Always`, and `Never`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.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
@@ -25429,6 +25429,73 @@
   verifyFormat("auto x = 5s .count() == 5;");
 }
 
+TEST_F(FormatTest, BreakBeforeParameterList) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.AlwaysBreakBeforeFunctionParameters, FormatStyle::FPBS_Leave);
+
+  // test Leave
+
+  // verify that there is no break by default
+  verifyFormat("int function1();\n" // formatted
+   "int function2(int param1, int param2, int param3);\n"
+   "void function3(int param1, int param2, int param3) {}\n"
+   "int function4(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function5(int param1, int param2, int param3);\n",
+   "int function1();\n" // original
+   "int function2(int param1, int param2, int param3);\n"
+   "void function3(int param1, int param2, int param3) {}\n"
+   "int function4(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function5(int param1, int param2, int param3);\n",
+   Style);
+
+  // test Always
+  // verify that there is a break when told to break
+  Style.AlwaysBreakBeforeFunctionParameters = FormatStyle::FPBS_Always;
+  verifyFormat("int function1(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function2();\n"
+   "void function3(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3) {}\n"
+   "int function4(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function5(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n",
+   Style);
+
+  // verify that having no parameters doesn't affect the parentheses
+  verifyFormat("void function1() {}\n", // the formatted part
+   "void function1() {}\n", // the original
+   Style);
+
+  verifyFormat("void function1();\n", // the formatted part
+   "void function1();\n", // the original
+   Style);
+
+  // test Never
+  Style.AlwaysBreakBeforeFunctionParameters = FormatStyle::FPBS_Never;
+  verifyFormat("int function1();\n" // the formatted part
+   "int function2(int param1, int param2, int param3);\n",
+   "int function1();\n" // the original
+   "int function2(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n",
+   Style);
+}
 } // namespace
 } // namespace test
 } // namespace format
Index: clang/unittests/Format/ConfigParseTest.cpp
===
--- clang/unittests/Format/ConfigParseTest.cpp
+++ clang/unittests/Format/ConfigParseTest.cpp
@@ -550,6 +550,14 @@
   CHECK_PARSE("AllowShortLambdasOnASingleLine: true",
   AllowShortLambdasOnASingleLine, FormatStyle::SLS_All);
 
+  Style.AlwaysBreakBeforeFunctionParameters = FormatStyle::FPBS_Never;
+  CHECK_PARSE("AlwaysBreakBeforeFunctionParameters: Leave",
+  AlwaysBreakBeforeFunctionParameters, FormatStyle::FPBS_Leave);
+  CHECK_PARSE("AlwaysBreakBeforeFunctionParameters: Always",
+  AlwaysBreakBeforeFunctionParameters, FormatStyle::FPBS_Always);
+  CHECK_PARSE("AlwaysBreakBeforeFunctionParameters: Never",
+  AlwaysBreakBeforeFunctionParameters, FormatStyle::FPBS_Never);
+
   Style.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_Both;
   CHECK_PARSE("SpaceAroundPointerQualifiers: Default",
   SpaceAroundPointerQualifiers, FormatStyle::SAPQ_Default);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -4840,6 +4840,22 @@
 return true;
   }
 
+  // If AlwaysBreakBeforeFunctionParameters is true, we want to break before
+  // the next parameter, if ther

[PATCH] D125171: [clang-format] Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin marked 4 inline comments as done.
jrmolin added inline comments.



Comment at: clang/lib/Format/Format.cpp:1336
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
+  LLVMStyle.AlwaysBreakBeforeFunctionParameters = false;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;

MyDeveloperDay wrote:
> ok, we have this think that the lifetime of an option goes from bool -> enum 
> -> struct, sometimes we pick up early that true/false aren't good enough
> 
> so here is the think... `AlwaysBreakBeforeFunctionParameters` should this be  
> an enum and `BreakBeforeFunctionParameters` but with values
> 
> `Leave, Never, Always`
> 
> i.e. does `AlwaysBreakBeforeFunctionParameters` = false mean never break? or 
> sometimes break.
> 
> We don't really want "false" to mean do something..we want it to mean don't 
> do anything i.e. Leave
> 
> 
> 
> 
Please let me know if I did this wrong! This took more than I was expecting. I 
added parsing for "false" and "true" to be "Leave" and "Always". That is kind 
of confusing, and there is no need for "backwards compatibility" in this, but 
it looked easy. Should I remove that?



Comment at: clang/unittests/Format/FormatTest.cpp:25437
+  // verify that there is no break by default
+  verifyFormat("int function1(int param1, int param2, int param3);\n"
+   "int function2();\n",

MyDeveloperDay wrote:
> I would say all these function could go to the single format of `verifyFormat`
I consolidated the tests and expanded.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

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

[PATCH] D125171: [clang-format] Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin updated this revision to Diff 513717.
jrmolin marked 2 inline comments as done.
jrmolin added a comment.

there were a couple of bugs in the last patch that I missed. format tests all 
pass again with this one.

I am using "Leave" as a transparent value, so no line breaks are added and none 
are removed -- the penalty calculus drives. Just like it was before this patch. 
It's the best I can do for making users not have to care about this option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.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
@@ -25429,6 +25429,66 @@
   verifyFormat("auto x = 5s .count() == 5;");
 }
 
+TEST_F(FormatTest, BreakBeforeParameterList) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.BreakBeforeFunctionParameters, FormatStyle::FPBS_Leave);
+  Style.BinPackParameters = false;
+
+  // test Leave (basically transparent mode)
+
+  // verify that there is no break by default
+  verifyFormat("int function1();\n" // formatted
+   "int function2(int param1, int param2, int param3);\n"
+   "void function3(int param1, int param2, int param3) {}\n"
+   "int function4(int param1, int param2, int param3);\n",
+   "int function1();\n" // original
+   "int function2(int param1, int param2, int param3);\n"
+   "void function3(int param1, int param2, int param3) {}\n"
+   "int function4(int param1, int param2, int param3);\n",
+   Style);
+
+  // test Always
+  // verify that there is a break when told to break
+  Style.BreakBeforeFunctionParameters = FormatStyle::FPBS_Always;
+  verifyFormat("int function1(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function2();\n"
+   "void function3(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3) {}\n"
+   "int function4(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function5(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n",
+   Style);
+
+  // verify that having no parameters doesn't affect the parentheses
+  verifyFormat("void function1() {}\n", // the formatted part
+   "void function1() {}\n", // the original
+   Style);
+
+  verifyFormat("void function1();\n", // the formatted part
+   "void function1();\n", // the original
+   Style);
+
+  // test Never
+  Style.BreakBeforeFunctionParameters = FormatStyle::FPBS_Never;
+  verifyFormat("int function1();\n" // the formatted part
+   "int function2(int param1, int param2, int param3);\n",
+   "int function1();\n" // the original
+   "int function2(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n",
+   Style);
+}
 } // namespace
 } // namespace test
 } // namespace format
Index: clang/unittests/Format/ConfigParseTest.cpp
===
--- clang/unittests/Format/ConfigParseTest.cpp
+++ clang/unittests/Format/ConfigParseTest.cpp
@@ -607,6 +607,14 @@
   CHECK_PARSE("BreakBeforeBraces: Custom", BreakBeforeBraces,
   FormatStyle::BS_Custom);
 
+  Style.BreakBeforeFunctionParameters = FormatStyle::FPBS_Never;
+  CHECK_PARSE("BreakBeforeFunctionParameters: Leave",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Leave);
+  CHECK_PARSE("BreakBeforeFunctionParameters: Always",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Always);
+  CHECK_PARSE("BreakBeforeFunctionParameters: Never",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Never);
+
   Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Never;
   CHECK_PARSE("BraceWrapping:\n"
   "  AfterControlStatement: MultiLine",
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -4840,6 +4840,21 @@
 return true;
   }
 
+  // If BreakBeforeFunctionParameters is Always, we want to break before
+  // the next parameter, if there is one. If it is Leave and a newline exists,
+  //

[PATCH] D125171: [clang-format] Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin updated this revision to Diff 515756.
jrmolin added a comment.

a unit test was failing, so I fixed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.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
@@ -25429,6 +25429,68 @@
   verifyFormat("auto x = 5s .count() == 5;");
 }
 
+TEST_F(FormatTest, BreakBeforeParameterList) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.BreakBeforeFunctionParameters, FormatStyle::FPBS_Leave);
+  Style.BinPackParameters = false;
+
+  // test Leave (basically transparent mode)
+
+  // verify that there is no break by default
+  verifyFormat("int function1();\n" // formatted
+   "int function2(int param1, int param2, int param3);\n"
+   "void function3(int param1, int param2, int param3) {}\n"
+   "int function4(int param1, int param2, int param3);\n",
+   "int function1();\n" // original
+   "int function2(\n"
+   "int param1, int param2, int param3);\n"
+   "void function3(int param1, int param2, int param3) {}\n"
+   "int function4(int param1, int param2, int param3);\n",
+   Style);
+
+  // test Always
+  // verify that there is a break when told to break
+  Style.BreakBeforeFunctionParameters = FormatStyle::FPBS_Always;
+  verifyFormat("int function1(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function2();\n"
+   "void function3(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3) {}\n"
+   "int function4(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function5(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n",
+   Style);
+
+  // verify that having no parameters doesn't affect the parentheses
+  verifyFormat("void function1() {}\n", // the formatted part
+   "void function1() {}\n", // the original
+   Style);
+
+  verifyFormat("void function1();\n", // the formatted part
+   "void function1();\n", // the original
+   Style);
+
+  // test Never
+  Style.BreakBeforeFunctionParameters = FormatStyle::FPBS_Never;
+  verifyFormat("int function1();\n" // the formatted part
+   "int function2(int param1, int param2, int param3);\n",
+   "int function1();\n" // the original
+   "int function2(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n",
+   Style);
+}
+
 TEST_F(FormatTest, InterfaceAsClassMemberName) {
   verifyFormat("class Foo {\n"
"  int interface;\n"
Index: clang/unittests/Format/ConfigParseTest.cpp
===
--- clang/unittests/Format/ConfigParseTest.cpp
+++ clang/unittests/Format/ConfigParseTest.cpp
@@ -607,6 +607,17 @@
   CHECK_PARSE("BreakBeforeBraces: Custom", BreakBeforeBraces,
   FormatStyle::BS_Custom);
 
+  CHECK_PARSE("BreakBeforeFunctionParameters: Always",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Always);
+  CHECK_PARSE("BreakBeforeFunctionParameters: Leave",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Leave);
+  CHECK_PARSE("BreakBeforeFunctionParameters: true",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Always);
+  CHECK_PARSE("BreakBeforeFunctionParameters: false",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Leave);
+  CHECK_PARSE("BreakBeforeFunctionParameters: Never",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Never);
+
   Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Never;
   CHECK_PARSE("BraceWrapping:\n"
   "  AfterControlStatement: MultiLine",
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -4867,6 +4867,21 @@
 return true;
   }
 
+  // If BreakBeforeFunctionParameters is Always, we want to break before
+  // the next parameter, if there is one. If it is Leave and a newline exists,
+  // make sure we insert one. Otherwise, no newline.
+  if (Left.is(tok::l_p

[PATCH] D125171: [clang-format] Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin added inline comments.



Comment at: clang/include/clang/Format/Format.h:851
+  /// The function parameter breaking style to use.
+  /// \version 16.0
+  FunctionParameterBreakingStyle BreakBeforeFunctionParameters;

so should this be 17.0 now? This has gone on quite long.



Comment at: clang/lib/Format/Format.cpp:345
+
+// For backward compatibility.
+IO.enumCase(Value, "false", FormatStyle::FPBS_Leave);

I should probably delete these, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

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

[PATCH] D125171: [clang-format] Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin updated this revision to Diff 516450.
jrmolin marked an inline comment as done.
jrmolin added a comment.

change formatting of enum options


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.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
@@ -25429,6 +25429,68 @@
   verifyFormat("auto x = 5s .count() == 5;");
 }
 
+TEST_F(FormatTest, BreakBeforeParameterList) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.BreakBeforeFunctionParameters, FormatStyle::FPBS_Leave);
+  Style.BinPackParameters = false;
+
+  // test Leave (basically transparent mode)
+
+  // verify that there is no break by default
+  verifyFormat("int function1();\n" // formatted
+   "int function2(int param1, int param2, int param3);\n"
+   "void function3(int param1, int param2, int param3) {}\n"
+   "int function4(int param1, int param2, int param3);\n",
+   "int function1();\n" // original
+   "int function2(\n"
+   "int param1, int param2, int param3);\n"
+   "void function3(int param1, int param2, int param3) {}\n"
+   "int function4(int param1, int param2, int param3);\n",
+   Style);
+
+  // test Always
+  // verify that there is a break when told to break
+  Style.BreakBeforeFunctionParameters = FormatStyle::FPBS_Always;
+  verifyFormat("int function1(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function2();\n"
+   "void function3(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3) {}\n"
+   "int function4(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function5(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n",
+   Style);
+
+  // verify that having no parameters doesn't affect the parentheses
+  verifyFormat("void function1() {}\n", // the formatted part
+   "void function1() {}\n", // the original
+   Style);
+
+  verifyFormat("void function1();\n", // the formatted part
+   "void function1();\n", // the original
+   Style);
+
+  // test Never
+  Style.BreakBeforeFunctionParameters = FormatStyle::FPBS_Never;
+  verifyFormat("int function1();\n" // the formatted part
+   "int function2(int param1, int param2, int param3);\n",
+   "int function1();\n" // the original
+   "int function2(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n",
+   Style);
+}
+
 TEST_F(FormatTest, InterfaceAsClassMemberName) {
   verifyFormat("class Foo {\n"
"  int interface;\n"
Index: clang/unittests/Format/ConfigParseTest.cpp
===
--- clang/unittests/Format/ConfigParseTest.cpp
+++ clang/unittests/Format/ConfigParseTest.cpp
@@ -607,6 +607,17 @@
   CHECK_PARSE("BreakBeforeBraces: Custom", BreakBeforeBraces,
   FormatStyle::BS_Custom);
 
+  CHECK_PARSE("BreakBeforeFunctionParameters: Always",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Always);
+  CHECK_PARSE("BreakBeforeFunctionParameters: Leave",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Leave);
+  CHECK_PARSE("BreakBeforeFunctionParameters: true",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Always);
+  CHECK_PARSE("BreakBeforeFunctionParameters: false",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Leave);
+  CHECK_PARSE("BreakBeforeFunctionParameters: Never",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Never);
+
   Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Never;
   CHECK_PARSE("BraceWrapping:\n"
   "  AfterControlStatement: MultiLine",
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -4867,6 +4867,21 @@
 return true;
   }
 
+  // If BreakBeforeFunctionParameters is Always, we want to break before
+  // the next parameter, if there is one. If it is Leave and a newline exists,
+  // make sure we insert one. Otherwise,

[PATCH] D125171: [clang-format] Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin updated this revision to Diff 516452.
jrmolin added a comment.

pulled upstream/main


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.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
@@ -25429,6 +25429,68 @@
   verifyFormat("auto x = 5s .count() == 5;");
 }
 
+TEST_F(FormatTest, BreakBeforeParameterList) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.BreakBeforeFunctionParameters, FormatStyle::FPBS_Leave);
+  Style.BinPackParameters = false;
+
+  // test Leave (basically transparent mode)
+
+  // verify that there is no break by default
+  verifyFormat("int function1();\n" // formatted
+   "int function2(int param1, int param2, int param3);\n"
+   "void function3(int param1, int param2, int param3) {}\n"
+   "int function4(int param1, int param2, int param3);\n",
+   "int function1();\n" // original
+   "int function2(\n"
+   "int param1, int param2, int param3);\n"
+   "void function3(int param1, int param2, int param3) {}\n"
+   "int function4(int param1, int param2, int param3);\n",
+   Style);
+
+  // test Always
+  // verify that there is a break when told to break
+  Style.BreakBeforeFunctionParameters = FormatStyle::FPBS_Always;
+  verifyFormat("int function1(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function2();\n"
+   "void function3(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3) {}\n"
+   "int function4(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function5(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n",
+   Style);
+
+  // verify that having no parameters doesn't affect the parentheses
+  verifyFormat("void function1() {}\n", // the formatted part
+   "void function1() {}\n", // the original
+   Style);
+
+  verifyFormat("void function1();\n", // the formatted part
+   "void function1();\n", // the original
+   Style);
+
+  // test Never
+  Style.BreakBeforeFunctionParameters = FormatStyle::FPBS_Never;
+  verifyFormat("int function1();\n" // the formatted part
+   "int function2(int param1, int param2, int param3);\n",
+   "int function1();\n" // the original
+   "int function2(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n",
+   Style);
+}
+
 TEST_F(FormatTest, InterfaceAsClassMemberName) {
   verifyFormat("class Foo {\n"
"  int interface;\n"
Index: clang/unittests/Format/ConfigParseTest.cpp
===
--- clang/unittests/Format/ConfigParseTest.cpp
+++ clang/unittests/Format/ConfigParseTest.cpp
@@ -607,6 +607,17 @@
   CHECK_PARSE("BreakBeforeBraces: Custom", BreakBeforeBraces,
   FormatStyle::BS_Custom);
 
+  CHECK_PARSE("BreakBeforeFunctionParameters: Always",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Always);
+  CHECK_PARSE("BreakBeforeFunctionParameters: Leave",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Leave);
+  CHECK_PARSE("BreakBeforeFunctionParameters: true",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Always);
+  CHECK_PARSE("BreakBeforeFunctionParameters: false",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Leave);
+  CHECK_PARSE("BreakBeforeFunctionParameters: Never",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Never);
+
   Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Never;
   CHECK_PARSE("BraceWrapping:\n"
   "  AfterControlStatement: MultiLine",
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -4867,6 +4867,21 @@
 return true;
   }
 
+  // If BreakBeforeFunctionParameters is Always, we want to break before
+  // the next parameter, if there is one. If it is Leave and a newline exists,
+  // make sure we insert one. Otherwise, no newline.
+  if (Left.is(tok::l_paren) && !Right.is(

[PATCH] D125171: [clang-format] Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin updated this revision to Diff 519293.
jrmolin added a comment.

more code review updates. pulled main.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.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
@@ -25655,6 +25655,68 @@
   verifyFormat("auto x = 5s .count() == 5;");
 }
 
+TEST_F(FormatTest, BreakBeforeParameterList) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.BreakBeforeFunctionParameters, FormatStyle::FPBS_Leave);
+  Style.BinPackParameters = false;
+
+  // test Leave (basically transparent mode)
+
+  // verify that there is no break by default
+  verifyFormat("int function1();\n" // formatted
+   "int function2(int param1, int param2, int param3);\n"
+   "void function3(int param1, int param2, int param3) {}\n"
+   "int function4(int param1, int param2, int param3);\n",
+   "int function1();\n" // original
+   "int function2(\n"
+   "int param1, int param2, int param3);\n"
+   "void function3(int param1, int param2, int param3) {}\n"
+   "int function4(int param1, int param2, int param3);\n",
+   Style);
+
+  // test Always
+  // verify that there is a break when told to break
+  Style.BreakBeforeFunctionParameters = FormatStyle::FPBS_Always;
+  verifyFormat("int function1(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function2();\n"
+   "void function3(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3) {}\n"
+   "int function4(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n"
+   "int function5(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n",
+   Style);
+
+  // verify that having no parameters doesn't affect the parentheses
+  verifyFormat("void function1() {}\n", // the formatted part
+   "void function1() {}\n", // the original
+   Style);
+
+  verifyFormat("void function1();\n", // the formatted part
+   "void function1();\n", // the original
+   Style);
+
+  // test Never
+  Style.BreakBeforeFunctionParameters = FormatStyle::FPBS_Never;
+  verifyFormat("int function1();\n" // the formatted part
+   "int function2(int param1, int param2, int param3);\n",
+   "int function1();\n" // the original
+   "int function2(\n"
+   "int param1,\n"
+   "int param2,\n"
+   "int param3);\n",
+   Style);
+}
+
 TEST_F(FormatTest, InterfaceAsClassMemberName) {
   verifyFormat("class Foo {\n"
"  int interface;\n"
Index: clang/unittests/Format/ConfigParseTest.cpp
===
--- clang/unittests/Format/ConfigParseTest.cpp
+++ clang/unittests/Format/ConfigParseTest.cpp
@@ -609,6 +609,13 @@
   CHECK_PARSE("BreakBeforeBraces: Custom", BreakBeforeBraces,
   FormatStyle::BS_Custom);
 
+  CHECK_PARSE("BreakBeforeFunctionParameters: Always",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Always);
+  CHECK_PARSE("BreakBeforeFunctionParameters: Leave",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Leave);
+  CHECK_PARSE("BreakBeforeFunctionParameters: Never",
+  BreakBeforeFunctionParameters, FormatStyle::FPBS_Never);
+
   Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Never;
   CHECK_PARSE("BraceWrapping:\n"
   "  AfterControlStatement: MultiLine",
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -4874,6 +4874,21 @@
 return true;
   }
 
+  // If BreakBeforeFunctionParameters is Always, we want to break before
+  // the next parameter, if there is one. If it is Leave and a newline exists,
+  // make sure we insert one. Otherwise, no newline.
+  if (Left.is(tok::l_paren) && !Right.is(tok::r_paren) && Left.Previous &&
+  Left.Previous->is(TT_FunctionDeclarationName)) {
+switch (Style.BreakBeforeFunctionParameters) {
+case FormatStyle::FPBS_Always:
+  return true;
+case FormatStyle::FPBS_Never:
+  

[PATCH] D125171: [clang-format] Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jonathan molinatto via Phabricator via cfe-commits]

jrmolin added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:25716
+   "int param2,\n"
+   "int param3);\n",
+   Style);

H-G-Hristov wrote:
> Does this work with `BAS_BlockIndent`? I don't see any related tests.
How do you want me to test it? I'm happy to, if that means this will get merged 
at some point. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

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