[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: 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;
+  

[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 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;
   

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:1374
+**AlwaysBreakBeforeFunctionParameters** (``Boolean``) 
:versionbadge:`clang-format 16.0` :ref:`¶ `
+  If ``true``, always break before function parameters in a declaration.
+

here you say declaration but don't we want the same in definition too? 
otherewise should it be `AlwaysBreakBeforeFunctionDefinitionParameters` if you 
are expecting something else?



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

This isn't relevant is it? `AlwaysBreakAfterReturnType`


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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Can you show your example from a implementation case and not just a declaration 
i.e. what happens with

`int function1(int param1) {}`




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)) {

can you test the `()` case in your tests please with 
`AlwaysBreakBeforeFunctionParameters` set to true


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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I don't like using additional variables in unit tests, if someone changes 25400 
multiple tests could break, each test should be stand alone in my view


Herald added a comment.

NOTE: Clang-Format Team Automated Review Comment

Your review contains a change to ClangFormatStyleOptions.rst but not a change 
to clang/include/clang/Format/Format.h

ClangFormatStyleOptions.rst is auto generated from Format.h via 
clang/docs/tools/dump_format_style.py,  please run this to regenerate the .rst

You can validate that the rst is valid by running.

  ./docs/tools/dump_format_style.py
  mkdir -p html
  /usr/bin/sphinx-build -n ./docs ./html




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);




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#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 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  = *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 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

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

Looks good to me, not giving my formal approval since the ongoing discussion 
about the style guide.




Comment at: clang/include/clang/Format/Format.h:823
+  /// \endcode
+  /// \version 16.0
+  bool AlwaysBreakBeforeFunctionParameters;

Since when do we add the `.0`?



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  = *Left.Previous;
+  if (TwoPrevious.is(TT_FunctionDeclarationName)) {
+  return true;
+  }
+  }

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.


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 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  = *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 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) &&

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

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks 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  = *Left.Previous;
+  if (TwoPrevious.is(TT_FunctionDeclarationName)) {
+  return true;
+  }
+  }

That I meant with merge.


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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Please rebase


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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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


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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



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

jrmolin wrote:
> 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.
that needs to be looked at, as to what changed that meant it changed and done 
separately.



Comment at: clang/docs/ClangFormatStyleOptions.rst:3699
+ 'A(a, b) will not be expanded.
+
 .. _MaxEmptyLinesToKeep:

This won't generate without a /version, we need to get this fixed outside of 
this commit



Comment at: clang/include/clang/Format/Format.h:823
+  /// \endcode
+  /// \version 16
+  bool AlwaysBreakBeforeFunctionParameters;

16.0


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 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 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  = *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 

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



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

jrmolin wrote:
> MyDeveloperDay wrote:
> > Regererate
> How do I re-generate? I'm happy to! I just don't see where to do that.
docs/tools/dump_format_style.py but ensure you move your example to Format.h 
first or its going to wipe out these changes


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#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

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

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"?


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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay requested changes to this revision.
MyDeveloperDay added inline comments.
This revision now requires changes to proceed.



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

Regererate



Comment at: clang/docs/ClangFormatStyleOptions.rst:1376
+
+  This flag is meant to align function parameters starting on the line 
following
+  a function declaration or definition. Thus, it will only take effect if a 
function

Ok you wrote this by hand it’s auto generated from format.h



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

Needs a parse test



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",

Please write it out 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 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  = *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) &&
 

[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 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

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

And please add an entry to the `ReleaseNotes.rst`.


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

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

Please reupload with the complete context.

Please add tests.




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

That's not a valid declaration (missing return type), so not a good example.



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

Please sort alphabetically. (So above `AlwaysBreakBeforeMultilineStrings`.)



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

HazardyKnusperkeks wrote:
> Please sort alphabetically. (So above `AlwaysBreakBeforeMultilineStrings`.)
You are missing the \since 17.



Comment at: clang/lib/Format/Format.cpp:1510
   GoogleStyle.AlwaysBreakBeforeMultilineStrings = true;
+  GoogleStyle.AlwaysBreakBeforeFunctionParameters = false;
   GoogleStyle.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_Yes;

Only set it in getLLVMStyle, which you are missing right now. Every other style 
will inherit it.



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) {

Merge the `if`s.


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

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

Please add unit tests to relevant files in `clang/unittests/Format/`.


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

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

See also 
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface.


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

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

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


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  = *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 

[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 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  = *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 

[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  = *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) ||
+