[clang-format] Allow break after return keyword

[gedare]

Summary

Currently new line breaks are not added between the return keyword
and the return value. With long, singleton return values, it can be
preferred to break before the return value. An example of this would be
a lengthy string return value.

Adds a new style option PenaltyBreakReturn to control when breaks are
preferred. With the current setting of 100, most existing unit tests pass.
One unit test needed to be tweaked, as it assumes very long return values
do not get broken from the return keyword. Added a new unit test to exercise
the long return value.

NOTE: Clang-Format Team Automated Review Comment

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

ClangFormatStyleOptions.rst is generated 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

[gedare]

Regenerate docs

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

NOTE: Clang-Format Team Automated Review Comment

It looks like your clang-format review does not contain any unit tests, please try to ensure all code changes have a unit test (unless this is an NFC or refactoring, adding documentation etc..)

Add your unit tests in clang/unittests/Format and you can build with ninja FormatTests. We recommend using the verifyFormat(xxx) format of unit tests rather than EXPECT_EQ as this will ensure you change is tolerant to random whitespace changes (see FormatTest.cpp as an example)

For situations where your change is altering the TokenAnnotator.cpp which can happen if you are trying to improve the annotation phase to ensure we are correctly identifying the type of a token, please add a token annotator test in TokenAnnotatorTest.cpp

[gedare]

• Regenerate ClangFormatStyleOptions.rst

[HazardyKnusperkeks]

I'd like more tests (and examples how it is before the change), some operations (+, *, etc.), your stated string literal with and without many spaces, call chains.

I have nothing against the introduction, I'm just not sure about the default value and the position of the check in splitPenalty.

And please add a remark in the changelog.

[gedare]

In D152975#4425932, @HazardyKnusperkeks wrote:

I'd like more tests (and examples how it is before the change), some operations (+, *, etc.), your stated string literal with and without many spaces, call chains.

Ok. Since it applies by default, all the existing unittests use it. I will see about fabricating some more interesting tests for it.

I have nothing against the introduction, I'm just not sure about the default value and the position of the check in splitPenalty.

Yes the default was chosen arbitrarily. Adding more tests to exercise return value lengths should help tune defaults.

And please add a remark in the changelog.

Is there a separate changelog file? I don't understand this request. Edit: I think this refers to ./clang/docs/ReleaseNotes.rst

[owenpan]

See D153205#4430528.

[HazardyKnusperkeks]

In D152975#4430504, @gedare wrote:

In D152975#4425932, @HazardyKnusperkeks wrote:

And please add a remark in the changelog.

Is there a separate changelog file? I don't understand this request. Edit: I think this refers to ./clang/docs/ReleaseNotes.rst

Yep that's the file.

[MyDeveloperDay]

clang/lib/Format/Format.cpp

1484

is 100 the current penalty?

clang/unittests/Format/FormatTest.cpp

22187

the fails the beyonce rule doesn't it, unless you are assuming the test is a bug?

[gedare]

I don't like the approach I took. It will be hard (impossible) to tune a default return penalty that ensures backward compatibility. The only way I see to make this work is to add an option to enable breaks after the return keyword. This is low on my priorities however, so I am abandoning this.

clang/docs/ClangFormatStyleOptions.rst

 **PenaltyBreakFirstLessLess** (``Unsigned``) :versionbadge:`clang-format 3.7` :ref:`¶ <PenaltyBreakFirstLessLess>`
   The penalty for breaking before the first ``<<``.
 
 .. _PenaltyBreakOpenParenthesis:
 
 **PenaltyBreakOpenParenthesis** (``Unsigned``) :versionbadge:`clang-format 14` :ref:`¶ <PenaltyBreakOpenParenthesis>`
   The penalty for breaking after ``(``.
 
+.. _PenaltyBreakReturn:
+
+**PenaltyBreakReturn** (``Unsigned``) :versionbadge:`clang-format 18` :ref:`¶ <PenaltyBreakReturn>`
+  The penalty for breaking after ``return``.
+
 .. _PenaltyBreakString:
 
 **PenaltyBreakString** (``Unsigned``) :versionbadge:`clang-format 3.7` :ref:`¶ <PenaltyBreakString>`
   The penalty for each line break introduced inside a string literal.
 
 .. _PenaltyBreakTemplateDeclaration:
 
 **PenaltyBreakTemplateDeclaration** (``Unsigned``) :versionbadge:`clang-format 7` :ref:`¶ <PenaltyBreakTemplateDeclaration>`

clang/include/clang/Format/Format.h

   /// The penalty for breaking before the first ``<<``.
   /// \version 3.7
   unsigned PenaltyBreakFirstLessLess;
 
   /// The penalty for breaking after ``(``.
   /// \version 14
   unsigned PenaltyBreakOpenParenthesis;
 
+  /// The penalty for breaking after ``return``.
+  /// \version 18
+  unsigned PenaltyBreakReturn;
+
   /// The penalty for each line break introduced inside a string literal.
   /// \version 3.7
   unsigned PenaltyBreakString;
 
   /// The penalty for breaking after template declaration.
   /// \version 7
   unsigned PenaltyBreakTemplateDeclaration;
 
return AccessModifierOffset == R.AccessModifierOffset &&
            ObjCSpaceBeforeProtocolList == R.ObjCSpaceBeforeProtocolList &&
            PackConstructorInitializers == R.PackConstructorInitializers &&
            PenaltyBreakAssignment == R.PenaltyBreakAssignment &&
            PenaltyBreakBeforeFirstCallParameter ==
                R.PenaltyBreakBeforeFirstCallParameter &&
            PenaltyBreakComment == R.PenaltyBreakComment &&
            PenaltyBreakFirstLessLess == R.PenaltyBreakFirstLessLess &&
            PenaltyBreakOpenParenthesis == R.PenaltyBreakOpenParenthesis &&
+           PenaltyBreakReturn == R.PenaltyBreakReturn &&
            PenaltyBreakString == R.PenaltyBreakString &&
            PenaltyBreakTemplateDeclaration ==
                R.PenaltyBreakTemplateDeclaration &&
            PenaltyExcessCharacter == R.PenaltyExcessCharacter &&
            PenaltyReturnTypeOnItsOwnLine == R.PenaltyReturnTypeOnItsOwnLine &&
            PointerAlignment == R.PointerAlignment &&
            QualifierAlignment == R.QualifierAlignment &&
            QualifierOrder == R.QualifierOrder &&

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("PenaltyBreakAssignment", Style.PenaltyBreakAssignment);
     IO.mapOptional("PenaltyBreakBeforeFirstCallParameter",
                    Style.PenaltyBreakBeforeFirstCallParameter);
     IO.mapOptional("PenaltyBreakComment", Style.PenaltyBreakComment);
     IO.mapOptional("PenaltyBreakFirstLessLess",
                    Style.PenaltyBreakFirstLessLess);
     IO.mapOptional("PenaltyBreakOpenParenthesis",
                    Style.PenaltyBreakOpenParenthesis);
+    IO.mapOptional("PenaltyBreakReturn", Style.PenaltyBreakReturn);
     IO.mapOptional("PenaltyBreakString", Style.PenaltyBreakString);
     IO.mapOptional("PenaltyBreakTemplateDeclaration",
                    Style.PenaltyBreakTemplateDeclaration);
     IO.mapOptional("PenaltyExcessCharacter", Style.PenaltyExcessCharacter);
     IO.mapOptional("PenaltyIndentedWhitespace",
                    Style.PenaltyIndentedWhitespace);
     IO.mapOptional("PenaltyReturnTypeOnItsOwnLine",
                    Style.PenaltyReturnTypeOnItsOwnLine);
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.WhitespaceSensitiveMacros.push_back("CF_SWIFT_NAME");
   LLVMStyle.WhitespaceSensitiveMacros.push_back("NS_SWIFT_NAME");
   LLVMStyle.WhitespaceSensitiveMacros.push_back("PP_STRINGIZE");
   LLVMStyle.WhitespaceSensitiveMacros.push_back("STRINGIZE");
 
   LLVMStyle.PenaltyBreakAssignment = prec::Assignment;
   LLVMStyle.PenaltyBreakComment = 300;
   LLVMStyle.PenaltyBreakFirstLessLess = 120;
+  LLVMStyle.PenaltyBreakReturn = 100;
   LLVMStyle.PenaltyBreakString = 1000;
   LLVMStyle.PenaltyExcessCharacter = 1000000;
   LLVMStyle.PenaltyReturnTypeOnItsOwnLine = 60;
   LLVMStyle.PenaltyBreakBeforeFirstCallParameter = 19;
   LLVMStyle.PenaltyBreakOpenParenthesis = 0;
   LLVMStyle.PenaltyBreakTemplateDeclaration = prec::Relational;
   LLVMStyle.PenaltyIndentedWhitespace = 0;
 

clang/lib/Format/TokenAnnotator.cpp

if (Right.is(TT_LambdaLSquare) && Left.is(tok::equal))
       return 35;
     if (!Right.isOneOf(TT_ObjCMethodExpr, TT_LambdaLSquare,
                        TT_ArrayInitializerLSquare,
                        TT_DesignatedInitializerLSquare, TT_AttributeSquare)) {
       return 500;
     }
   }
 
+  if (Left.is(tok::kw_return))
+    return Style.PenaltyBreakReturn;
   if (Left.is(tok::coloncolon))
     return 500;
   if (Right.isOneOf(TT_StartOfName, TT_FunctionDeclarationName) ||
       Right.is(tok::kw_operator)) {
     if (Line.startsWith(tok::kw_for) && Right.PartOfMultiVariableDeclStmt)
       return 3;
     if (Left.is(TT_StartOfName))
       return 110;
bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
   if (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace)) {
     if (isAllmanLambdaBrace(Left))
       return !isItAnEmptyLambdaAllowed(Left, ShortLambdaOption);
     if (isAllmanLambdaBrace(Right))
       return !isItAnEmptyLambdaAllowed(Right, ShortLambdaOption);
   }
 
   return Left.isOneOf(tok::comma, tok::coloncolon, tok::semi, tok::l_brace,
-                      tok::kw_class, tok::kw_struct, tok::comment) ||
+                      tok::kw_class, tok::kw_struct, tok::comment,
+                      tok::kw_return) ||
          Right.isMemberAccess() ||
          Right.isOneOf(TT_TrailingReturnArrow, TT_LambdaArrow, tok::lessless,
                        tok::colon, tok::l_square, tok::at) ||
          (Left.is(tok::r_paren) &&
           Right.isOneOf(tok::identifier, tok::kw_const)) ||
          (Left.is(tok::l_paren) && !Right.is(tok::r_paren)) ||
          (Left.is(TT_TemplateOpener) && !Right.is(TT_TemplateCloser));
 }

clang/unittests/Format/FormatTest.cpp

verifyFormat(
       "return (aaaaaaaaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa >=\n"
       "        aaaaaaaaaaaaaaaaaaaaaa());");
   verifyFormat("return aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
                "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);");
   verifyFormat("return aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
                "           aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) &&\n"
                "       aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa;");
   verifyFormat("return\n"
+               "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+               "aaaaaaaaaaaa();");
+  verifyFormat("return\n"
                "    // true if code is one of a or b.\n"
                "    code == a || code == b;");
 }
 
 TEST_F(FormatTest, AlignsAfterOpenBracket) {
   verifyFormat(
       "void aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaa aaaaaaaa,\n"
       "                                                aaaaaaaaa aaaaaaa) {}");
verifyFormat(
       "                                SecondParam,\n"
       "                                ThirdParam,\n"
       "                                FourthParam,\n"
       "                                []() { return SomeValueNotSoLong; });",
       LLVMWithBeforeLambdaBody);
   verifyFormat("FctWithLongLineInLambda_SLS_All(\n"
                "    []()\n"
                "    {\n"
-               "      return "
-               "HereAVeryLongLineThatWillBeFormattedOnMultipleLineAndShouldNotB"
-               "eConsiderAsInline;\n"
+               "      return\n"
+               "          HereAVeryLongLineThatWillBeFormattedOnMultipleLineAnd"
+               "ShouldNotBeConsiderAsInline;\n"
                "    });",
                LLVMWithBeforeLambdaBody);
   verifyFormat(
       "FctWithLongLineInLambda_SLS_All(\n"
       "    []()\n"
       "    {\n"
       "      return HereAVeryLongLine(ThatWillBeFormatted, OnMultipleLine,\n"
       "                               AndShouldNotBeConsiderAsInline,\n"