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

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

@jrmolin is this option really for breaking before the first parameter? Or are you trying to have one parameter per line as shown by the examples in your GitHub issue? Also, how would it interact with `BAS_AlwaysBreak`, `BinPackParameters`, and `AllowAllParametersOfDeclarationOnNextLine`? What if there is only one parameter? And do we really want something like the following for example?

  template <typename T>
  void baz(
      T t) requires C<T>



================
Comment at: clang/include/clang/Format/Format.h:812-814
+    /// Be transparent with line breaks before function parameters. This allows
+    /// the
+    /// penalty calculations to drive line breaks.
----------------



================
Comment at: clang/include/clang/Format/Format.h:816
+    /// \code
+    ///    int someFunction(int arg1, int arg2);
+    /// \endcode
----------------
Can you add an example to show that the default penalty would break before the first parameter?


================
Comment at: clang/include/clang/Format/Format.h:837
+  /// The function parameter breaking style to use.
+  /// \version 17.0
+  FunctionParameterBreakingStyle BreakBeforeFunctionParameters;
----------------



================
Comment at: clang/lib/Format/ContinuationIndenter.cpp:357
   }
+
   if (Current.MustBreakBefore ||
----------------
Unrelated?


================
Comment at: clang/lib/Format/ContinuationIndenter.cpp:584
+  // Check if we want to break before the first function parameter in
+  // declarations and definitions
+  if (startsNextParameter(Current, Style) && State.Line->MustBeDeclaration &&
----------------



================
Comment at: clang/lib/Format/ContinuationIndenter.cpp:586
+  if (startsNextParameter(Current, Style) && State.Line->MustBeDeclaration &&
+      Current.Previous->is(tok::l_paren)) {
+    switch (Style.BreakBeforeFunctionParameters) {
----------------
What would happen if there‘s a comment between the `l_paren` and the parameter?


================
Comment at: clang/lib/Format/ContinuationIndenter.cpp:1073-1075
+    // this is a dict/object literal. Break if
+    // BreakBeforeFunctionParameters is Always and it's a function
+    // declaration or if it's Leave and a newline exists already.
----------------
Please reflow the comments.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:4877-4879
+  // 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.
----------------
Reflow comments.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:4880
+  // 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)) {
----------------
Again, we need to handle the case where a comment exists between the `l_paren` and the parameter.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:4882-4890
+    switch (Style.BreakBeforeFunctionParameters) {
+    case FormatStyle::FPBS_Always:
+      return true;
+    case FormatStyle::FPBS_Never:
+      return false;
+    case FormatStyle::FPBS_Leave:
+      break;
----------------
Can you refactor the `switch` statement as it's also used in ContinuationIndenter.cpp above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171