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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.


================
Comment at: clang/lib/Format/Format.cpp:876
                    Style.AlwaysBreakBeforeMultilineStrings);
+    IO.mapOptional("AlwaysBreakBeforeFunctionParameters",
+                   Style.AlwaysBreakBeforeFunctionParameters);
----------------
This should be Alphabetic should this be before the MultlineStrings option?


================
Comment at: clang/lib/Format/Format.cpp:1336
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
+  LLVMStyle.AlwaysBreakBeforeFunctionParameters = false;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
----------------
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






================
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",
----------------
I would say all these function could go to the single format of `verifyFormat`


================
Comment at: clang/unittests/Format/FormatTest.cpp:25469
+  verifyFormat("void function1() {}\n", // the formatted part
+               "void function1() {}\n", // the original
+               Style);
----------------
you can just use the single form of verifyFormat() it will call messUp to remove the whitespace


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171