Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang-format] Add BreakFunctionDefinitionParameters option #84988

Merged
merged 3 commits into from
Apr 2, 2024

Conversation

ameerj
Copy link
Contributor

@ameerj ameerj commented Mar 12, 2024

This adds an option to break function definition parameters, putting them on the next line after the function's opening paren.

This was a missing step towards allowing styles which require all function definition parameters be on their own lines.

Closes #62963

Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang-format labels Mar 12, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 12, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-format

Author: Ameer J (ameerj)

Changes

This adds an option to break function definition parameters, putting them on the next line after the function's opening paren.

This was a missing step towards allowing styles which require all function definition parameters be on their own lines.

Closes #62963


Full diff: https://github.com/llvm/llvm-project/pull/84988.diff

6 Files Affected:

  • (modified) clang/docs/ClangFormatStyleOptions.rst (+6)
  • (modified) clang/include/clang/Format/Format.h (+7)
  • (modified) clang/lib/Format/Format.cpp (+3)
  • (modified) clang/lib/Format/FormatToken.h (+3)
  • (modified) clang/lib/Format/TokenAnnotator.cpp (+8)
  • (modified) clang/unittests/Format/FormatTest.cpp (+18)
diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst
index 5b00a8f4c00fb8..a5e710e21c8338 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -3150,6 +3150,12 @@ the configuration (without a prefix: ``Auto``).
 
 
 
+.. _BreakFunctionDefinitionParameters:
+
+**BreakFunctionDefinitionParameters** (``Boolean``) :versionbadge:`clang-format 19` :ref:`¶ <BreakFunctionDefinitionParameters>`
+  If ``true``, clang-format will always break before function definition
+  parameters
+
 .. _BreakInheritanceList:
 
 **BreakInheritanceList** (``BreakInheritanceListStyle``) :versionbadge:`clang-format 7` :ref:`¶ <BreakInheritanceList>`
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index 590297fd89a398..eb115dfd2b275d 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -2218,6 +2218,11 @@ struct FormatStyle {
   /// \version 3.8
   bool BreakAfterJavaFieldAnnotations;
 
+  /// If ``true``, clang-format will always break before function definition
+  /// parameters
+  /// \version 19
+  bool BreakFunctionDefinitionParameters;
+
   /// Allow breaking string literals when formatting.
   ///
   /// In C, C++, and Objective-C:
@@ -4867,6 +4872,8 @@ struct FormatStyle {
            BreakBeforeInlineASMColon == R.BreakBeforeInlineASMColon &&
            BreakBeforeTernaryOperators == R.BreakBeforeTernaryOperators &&
            BreakConstructorInitializers == R.BreakConstructorInitializers &&
+           BreakFunctionDefinitionParameters ==
+               R.BreakFunctionDefinitionParameters &&
            BreakInheritanceList == R.BreakInheritanceList &&
            BreakStringLiterals == R.BreakStringLiterals &&
            BreakTemplateDeclarations == R.BreakTemplateDeclarations &&
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index e64ba7eebc1ce8..ebdb4eae9520d3 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -961,6 +961,8 @@ template <> struct MappingTraits<FormatStyle> {
     IO.mapOptional("BreakAfterJavaFieldAnnotations",
                    Style.BreakAfterJavaFieldAnnotations);
     IO.mapOptional("BreakAfterReturnType", Style.BreakAfterReturnType);
+    IO.mapOptional("BreakFunctionDefinitionParameters",
+                   Style.BreakFunctionDefinitionParameters);
     IO.mapOptional("BreakArrays", Style.BreakArrays);
     IO.mapOptional("BreakBeforeBinaryOperators",
                    Style.BreakBeforeBinaryOperators);
@@ -1471,6 +1473,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.BreakAfterAttributes = FormatStyle::ABS_Leave;
   LLVMStyle.BreakAfterJavaFieldAnnotations = false;
   LLVMStyle.BreakAfterReturnType = FormatStyle::RTBS_None;
+  LLVMStyle.BreakFunctionDefinitionParameters = false;
   LLVMStyle.BreakArrays = true;
   LLVMStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
   LLVMStyle.BreakBeforeBraces = FormatStyle::BS_Attach;
diff --git a/clang/lib/Format/FormatToken.h b/clang/lib/Format/FormatToken.h
index 31245495041960..b20682de91c0b7 100644
--- a/clang/lib/Format/FormatToken.h
+++ b/clang/lib/Format/FormatToken.h
@@ -571,6 +571,9 @@ struct FormatToken {
   /// Is optional and can be removed.
   bool Optional = false;
 
+  /// Might be function declaration open/closing paren.
+  bool MightBeFunctionDeclParen = false;
+
   /// Number of optional braces to be inserted after this token:
   ///   -1: a single left brace
   ///    0: no braces
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index 04f0374b674e53..3d1c87d0b7253c 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -1501,6 +1501,8 @@ class AnnotatingParser {
             (!Previous->isAttribute() &&
              !Previous->isOneOf(TT_RequiresClause, TT_LeadingJavaAnnotation))) {
           Line.MightBeFunctionDecl = true;
+          Tok->MightBeFunctionDeclParen = true;
+          Tok->MatchingParen->MightBeFunctionDeclParen = true;
         }
       }
       break;
@@ -5313,6 +5315,12 @@ bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line,
   if (Right.NewlinesBefore > 1 && Style.MaxEmptyLinesToKeep > 0)
     return true;
 
+  if (Style.BreakFunctionDefinitionParameters && Line.MightBeFunctionDecl &&
+      Line.mightBeFunctionDefinition() && Left.is(tok::l_paren) &&
+      Left.MightBeFunctionDeclParen && Left.ParameterCount > 0) {
+    return true;
+  }
+
   if (Style.isCSharp()) {
     if (Left.is(TT_FatArrow) && Right.is(tok::l_brace) &&
         Style.BraceWrapping.AfterFunction) {
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index fc367a7a5a8981..d70a3c916a2d75 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -7951,6 +7951,24 @@ TEST_F(FormatTest, AllowAllArgumentsOnNextLineDontAlign) {
                Input, Style);
 }
 
+TEST_F(FormatTest, BreakFunctionDefinitionParameters) {
+  FormatStyle Style = getLLVMStyleWithColumns(80);
+  StringRef Input = "void functionDecl(paramA, paramB, paramC);\n"
+                    "void emptyFunctionDefinition() {}\n"
+                    "void functionDefinition(int A, int B, int C) {}";
+  Style.BreakFunctionDefinitionParameters = false;
+  verifyFormat(StringRef("void functionDecl(paramA, paramB, paramC);\n"
+                         "void emptyFunctionDefinition() {}\n"
+                         "void functionDefinition(int A, int B, int C) {}"),
+               Input, Style);
+  Style.BreakFunctionDefinitionParameters = true;
+  verifyFormat(StringRef("void functionDecl(paramA, paramB, paramC);\n"
+                         "void emptyFunctionDefinition() {}\n"
+                         "void functionDefinition(\n"
+                         "    int A, int B, int C) {}"),
+               Input, Style);
+}
+
 TEST_F(FormatTest, BreakBeforeInlineASMColon) {
   FormatStyle Style = getLLVMStyle();
   Style.BreakBeforeInlineASMColon = FormatStyle::BBIAS_Never;

Copy link

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

@ameerj
Copy link
Contributor Author

ameerj commented Mar 25, 2024

@HazardyKnusperkeks @owenca I'd like to request a review please

clang/include/clang/Format/Format.h Outdated Show resolved Hide resolved
clang/include/clang/Format/Format.h Outdated Show resolved Hide resolved
clang/lib/Format/TokenAnnotator.cpp Show resolved Hide resolved
clang/unittests/Format/FormatTest.cpp Outdated Show resolved Hide resolved
@ameerj
Copy link
Contributor Author

ameerj commented Mar 28, 2024

@HazardyKnusperkeks Thanks for the review!

I don't have repo permissions so I can't run all CI workflows or merge the PR.

@HazardyKnusperkeks HazardyKnusperkeks merged commit 13be0d4 into llvm:main Apr 2, 2024
5 checks passed
Copy link

github-actions bot commented Apr 2, 2024

@ameerj Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

@owenca owenca removed the clang Clang issues not falling into any other category label Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clang-format: Add config option to allow function parameters to be displayed on separate lines
4 participants