clang-format: add option to merge empty function body

[Typz]

Summary

This option supplements the AllowShortFunctionsOnASingleLine flag, to
merge empty function body at the beginning of the line: e.g. when the
function is not short-enough and breaking braces after function.

int f()
{}

[Typz]

unittests/Format/FormatTest.cpp

6029

When testing this patch, I found a bug when AllowShortFunctionsOnASingleLine = SFS_Empty and MergeEmptyOnly.BraceWrapping.AfterFunction = true : I did not have the time to fix it yet, any idea what might be causing this?

[Typz]

include/clang/Format/Format.h

158

maybe this should be a nested option inside BraceWrapping?

or this should be donc implicit when breaking after function (BraceWrapping.AfterFunction = true) and AllowShortFunctionOnASingleLine (hence no option added) ? Or even AllowShortBlocksOnASingleLine (though the doc limits this option to statements...)

unittests/Format/FormatTest.cpp

6131

missing test for parsing YAML option

[Typz]

Fix SFS_Empty & SFS_Inline merging of empty function when BraceWrapping.AfterFunction, and add missing test

[djasper]

Does anything speak against making this behavior happen with AllowShortFunctionsOnASingleLine = SFS_Empty and MergeEmptyOnly.BraceWrapping.AfterFunction = true? I mean without the extra style option?

unittests/Format/FormatTest.cpp

6063

indent

[Typz]

In D33447#763045, @djasper wrote:

Does anything speak against making this behavior happen with AllowShortFunctionsOnASingleLine = SFS_Empty and MergeEmptyOnly.BraceWrapping.AfterFunction = true? I mean without the extra style option?

That is fine with me (with AllowShortFunctionsOnASingleLine >= SFS_Empty condition), but I see two things:

• it does not match the "OnASingleLine" part of that option's name, since in that case we get the function definition on one line, and the body on the next line
• some (existing) coding style may use BraceWrapping.AfterFunction and AllowShortFunctionsOnASingleLine (for exemple Mozilla), but may not want to change the behavior when the function cannot be merged on a single line

[Typz]

fix indent

[djasper]

As it currently stands, I am really not happy with the configuration space that this opens up. If we can't make the configuration of existing flags, what's the coding style encourages this behavior?

[Typz]

Facebook's HHVM seems to use this for empty constructors: https://github.com/facebook/hhvm/blob/master/hphp/doc/coding-conventions.md
This is also done systematically in Qt and QtCreator code.

Personally, I don't care if this is an option or not; but I fear it would break existing styles.

[djasper]

But that style specifically says that it is only done if the initializer list is wrapped:
https://github.com/facebook/hhvm/blob/master/hphp/doc/coding-conventions.md#constructor-initializer-lists

I.e. we would do the right thing for that style if we would set BraceWrapping.AfterFunction to true and AllowShortFunctionsOnASingleLine to SFS_Empty and it would then format:

Constructor() {}
Constructor() //
    : initializer(..)
{}

[Typz]

I am fine with "removing" the options: I only fear it would incorrectly affect existing users/styles, which would possibly end up in the patch being reverted...
I could not find the info in Mozilla coding style, but looking at the code it seems it should be done indeed, but with an extra space in between, like this:

void foo()
{ }

So if you tell me it's fine I will go ahead and replace the option with the BraceWrapping.AfterFunction && AllowShortFunctionsOnASingleLine >= SFS_Empty condition.

[djasper]

No, I don't think it should be done this way and neither Facebook nor Mozilla coding styles say you should.

Mozilla style has an explicit example:

int TinyFunction() { return mVar; }

Facebook style has an explicit example:

MyClass::MyClass(uint64_t idx) : m_idx(idx) {}

Moving the "{}" to the next line is only ok if the complete function/constructor definition (up to and including the "}") does not fit on one line.

[Typz]

Right, I was not clear enough: this is indeed done only if everything does not fit on one line.

There is an exemple in facebook's coding style:

MyClass::MyClass(const Class* cls, const Func* func, const Class* ctx)
  : m_cls(cls)
  , m_func(func)
  , m_ctx(ctx)
  , m_isMyConditionMet(false)
{}

And this happens (with the extra space) in the same kind of situation in Mozilla's code:

nsSameProcessAsyncMessageBase::nsSameProcessAsyncMessageBase(JS::RootingContext* aRootingCx,
                                                             JS::Handle<JSObject*> aCpows)
  : mRootingCx(aRootingCx)
  , mCpows(aRootingCx, aCpows)
{ }

[Typz]

Merging empty blocks is not allowed in webkit style: https://webkit.org/code-style-guidelines/#punctuation-member-init

So it seems changing the behavior would actually fix mozilla style and break webkit style (as integrated in clang-format).

[djasper]

I don't understand. WebKit style would not set AllowShortFunctionsOnASingleLine and so the behavior there wouldn't change, I presume?

[Typz]

Webkit inherits AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All from LLVM style, so merging the options would introduce these explicitely-forbidden empty blocks.
But the empty blocks should actually be used in code following Mozilla or Qt style.

[Typz]

update mozilla style to use empty function blocks

[djasper]

I think it's just wrong that WebKit inherits this. The style guide explicitly says that this is wrong:

MyOtherClass::MyOtherClass() : MySuperClass() {}

So I still think we can make this work with the existing options without regressing a behavior that anyone is relying on.

[Typz]

In D33447#765610, @djasper wrote:

I think it's just wrong that WebKit inherits this. The style guide explicitly says that this is wrong:

MyOtherClass::MyOtherClass() : MySuperClass() {}

I think this exemple applies to constructors only (and it works even with inlining, since this style has BreakConstructorInitializersBeforeComma = true and ConstructorInitializerAllOnOneLineOrOnePerLine = false, so there is already a line break before the colon)

So I still think we can make this work with the existing options without regressing a behavior that anyone is relying on.

Looking at webkit code, there are many short functions which are indeed formatted on a single line (at least 22097 occurences, actually), so removing AllowShortFunctionsOnASingleLine from webkit style does not seem appropriate.

[djasper]

Lets try this the other way around. I am not ok with introducing an additional top-level option for this. It simply isn't important enough. So find a way for the existing style flags to support what you need and not regress existing users. If that can't be done, I am also ok with adding another value into BraceWrapping (which you suggested at some point, I think).

[Typz]

move option to BraceWrapping

[djasper]

include/clang/Format/Format.h

657

Reflow the comment.

667

I think the name probably isn't very intuitive. Maybe invert it and call it "SplitEmptyFunctionBody"?

unittests/Format/FormatTest.cpp

6088

indent. Here and elsewhere.

[Typz]

fix indent & rename option to SplitEmptyFunctionBody

[Typz]

ping

[Typz]

ping?

[djasper]

Looks good.

include/clang/Format/Format.h

struct FormatStyle {
   /// \code
   ///   true:                                   false:
   ///   myFunction(foo,                 vs.     myFunction(foo, bar, plop);
   ///              bar,
   ///              plop);
   /// \endcode
   bool AllowAllParametersOfDeclarationOnNextLine;
 
+  /// \brief If ``true``, empty function body can be put on a single line.
+  /// This option is used only if the opening brace of the function has already
+  /// been wrapped, i.e. if clang-format failed to put the function on a single
+  /// line (as per `AllowShortFunctionsOnASingleLine`) and if `AfterFunction`
+  /// brace wrapping mode is set.
+  /// \code
+  ///   int f()   vs.   inf f()
+  ///   {}              {
+  ///                   }
+  /// \endcode
+  ///
+  bool AllowEmptyFunctionBodyOnASingleLine;
+
   /// \brief Allows contracting simple braced statements to a single line.
   ///
   /// E.g., this allows ``if (a) { return; }`` to be put on a single line.
   bool AllowShortBlocksOnASingleLine;
 
   /// \brief If ``true``, short case labels will be contracted to a single line.
   /// \code
   ///   true:                                   false:
struct BraceWrappingFlags {
     ///   false:
     ///   union foo {
     ///     int x;
     ///   }
     /// \endcode
     bool AfterUnion;
     /// \brief Wrap before ``catch``.
     /// \code
     ///   true:
     ///   try {
     ///     foo();
     ///   }
     ///   catch () {
     ///   }
     ///
     ///   false:
     ///   try {
     ///     foo();
     ///   } catch () {
     ///   }
     /// \endcode
     bool BeforeCatch;
     /// \brief Wrap before ``else``.
     /// \code
     ///   true:
     ///   if (foo()) {
     ///   }
return AccessModifierOffset == R.AccessModifierOffset &&
            AlignAfterOpenBracket == R.AlignAfterOpenBracket &&
            AlignConsecutiveAssignments == R.AlignConsecutiveAssignments &&
            AlignConsecutiveDeclarations == R.AlignConsecutiveDeclarations &&
            AlignEscapedNewlinesLeft == R.AlignEscapedNewlinesLeft &&
            AlignOperands == R.AlignOperands &&
            AlignTrailingComments == R.AlignTrailingComments &&
            AllowAllParametersOfDeclarationOnNextLine ==
                R.AllowAllParametersOfDeclarationOnNextLine &&
+           AllowEmptyFunctionBodyOnASingleLine ==
+               R.AllowEmptyFunctionBodyOnASingleLine &&
            AllowShortBlocksOnASingleLine == R.AllowShortBlocksOnASingleLine &&
            AllowShortCaseLabelsOnASingleLine ==
                R.AllowShortCaseLabelsOnASingleLine &&
            AllowShortFunctionsOnASingleLine ==
                R.AllowShortFunctionsOnASingleLine &&
            AllowShortIfStatementsOnASingleLine ==
                R.AllowShortIfStatementsOnASingleLine &&
            AllowShortLoopsOnASingleLine == R.AllowShortLoopsOnASingleLine &&

lib/Format/Format.cpp

IO.mapOptional("AlignConsecutiveAssignments",
                    Style.AlignConsecutiveAssignments);
     IO.mapOptional("AlignConsecutiveDeclarations",
                    Style.AlignConsecutiveDeclarations);
     IO.mapOptional("AlignEscapedNewlinesLeft", Style.AlignEscapedNewlinesLeft);
     IO.mapOptional("AlignOperands", Style.AlignOperands);
     IO.mapOptional("AlignTrailingComments", Style.AlignTrailingComments);
     IO.mapOptional("AllowAllParametersOfDeclarationOnNextLine",
                    Style.AllowAllParametersOfDeclarationOnNextLine);
+    IO.mapOptional("AllowEmptyFunctionBodyOnASingleLine",
+                   Style.AllowEmptyFunctionBodyOnASingleLine);
     IO.mapOptional("AllowShortBlocksOnASingleLine",
                    Style.AllowShortBlocksOnASingleLine);
     IO.mapOptional("AllowShortCaseLabelsOnASingleLine",
                    Style.AllowShortCaseLabelsOnASingleLine);
     IO.mapOptional("AllowShortFunctionsOnASingleLine",
                    Style.AllowShortFunctionsOnASingleLine);
     IO.mapOptional("AllowShortIfStatementsOnASingleLine",
                    Style.AllowShortIfStatementsOnASingleLine);
FormatStyle getLLVMStyle() {
   LLVMStyle.AccessModifierOffset = -2;
   LLVMStyle.AlignEscapedNewlinesLeft = false;
   LLVMStyle.AlignAfterOpenBracket = FormatStyle::BAS_Align;
   LLVMStyle.AlignOperands = true;
   LLVMStyle.AlignTrailingComments = true;
   LLVMStyle.AlignConsecutiveAssignments = false;
   LLVMStyle.AlignConsecutiveDeclarations = false;
   LLVMStyle.AllowAllParametersOfDeclarationOnNextLine = true;
+  LLVMStyle.AllowEmptyFunctionBodyOnASingleLine = false;
   LLVMStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
   LLVMStyle.AllowShortBlocksOnASingleLine = false;
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
   LLVMStyle.AllowShortIfStatementsOnASingleLine = false;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;

lib/Format/UnwrappedLineFormatter.cpp

if (I[1]->Type == LT_Invalid || I[1]->First->MustBreakBefore)
       return 0;
     if (TheLine->InPPDirective &&
         (!I[1]->InPPDirective || I[1]->First->HasUnescapedNewline))
       return 0;
 
     if (Style.ColumnLimit > 0 && Indent > Style.ColumnLimit)
       return 0;
 
+    if (TheLine->Last->is(TT_FunctionLBrace) &&
+        TheLine->First == TheLine->Last &&
+        Style.AllowEmptyFunctionBodyOnASingleLine &&
+        I[1]->First->is(tok::r_brace))
+      return tryMergeSimpleBlock(I, E, 1);
+
     unsigned Limit =
         Style.ColumnLimit == 0 ? UINT_MAX : Style.ColumnLimit - Indent;
     // If we already exceed the column limit, we set 'Limit' to 0. The different
     // tryMerge..() functions can then decide whether to still do merging.
     Limit = TheLine->Last->TotalLength > Limit
                 ? 0
                 : Limit - TheLine->Last->TotalLength;
 

unittests/Format/FormatTest.cpp

verifyFormat("#define A            \\\n"
                "  int j;",
                getLLVMStyleWithColumns(22));
   verifyFormat("#define A             \\\n"
                "  void f() { int i; } \\\n"
                "  int j;",
                getLLVMStyleWithColumns(23));
 }
 
+TEST_F(FormatTest, PullEmptyFunctionDefinitionsIntoSingleLine) {
+  FormatStyle MergeEmptyOnly = getLLVMStyle();
+  MergeEmptyOnly.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty;
+  verifyFormat("class C {\n"
+               "  int f() {}\n"
+               "};",
+               MergeEmptyOnly);
+  verifyFormat("class C {\n"
+               "  int f() {\n"
+         "    return 42;\n"
+         "  }\n"
+               "};",
+               MergeEmptyOnly);
+  verifyFormat("int f() {}",
+               MergeEmptyOnly);
+  verifyFormat("int f() {\n"
+               "  return 42;\n"
+               "}",
+               MergeEmptyOnly);
+
+  //TODO: this case does not work, not sure if there is reason...
+  MergeEmptyOnly.BreakBeforeBraces = FormatStyle::BS_Custom;
+  MergeEmptyOnly.BraceWrapping.AfterFunction = true;
+  verifyFormat("int f() {}", MergeEmptyOnly);
+}
+
 TEST_F(FormatTest, PullInlineFunctionDefinitionsIntoSingleLine) {
   FormatStyle MergeInlineOnly = getLLVMStyle();
   MergeInlineOnly.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
   verifyFormat("class C {\n"
                "  int f() { return 42; }\n"
                "};",
                MergeInlineOnly);
   verifyFormat("int f() {\n"
                "  return 42;\n"
                "}",
                MergeInlineOnly);
+
+  // SFS_Inline implies SFS_Empty
+  verifyFormat("class C {\n"
+               "  int f() {}\n"
+               "};",
+               MergeInlineOnly);
+  verifyFormat("int f() {}",
+               MergeInlineOnly);
+}
+
+TEST_F(FormatTest, AllowEmptyFunctionBodyOnASingleLine) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
+  Style.AllowEmptyFunctionBodyOnASingleLine = true;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterFunction = true;
+  Style.ColumnLimit = 40;
+
+  verifyFormat("int f()\n"
+               "{}",
+               Style);
+  verifyFormat("int f()\n"
+         "{\n"
+               "  return 42;\n"
+               "}",
+               Style);
+  verifyFormat("int f()\n"
+         "{\n"
+               "  // some comment\n"
+               "}",
+               Style);
+
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty;
+  //verifyFormat("int f() {}", Style); //TODO: re-enable once PullEmptyFunctionDefinitionsIntoSingleLine is fixed
+  verifyFormat("int aaaaaaaaaaaaaa(int bbbbbbbbbbbbbb)\n"
+         "{}",
+               Style);
+  verifyFormat("int f()\n"
+         "{\n"
+         "  return 0;\n"
+         "}",
+               Style);
+
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
+  verifyFormat("class Foo {\n"
+         "  int f() {}\n"
+         "};\n",
+               Style);
+    verifyFormat("class Foo {\n"
+         "  int f() { return 0; }\n"
+         "};\n",
+               Style);
+  verifyFormat("class Foo {\n"
+         "  int aaaaaaaaaaaaaa(int bbbbbbbbbbbbbb)\n"
+         "  {}\n"
+         "};\n",
+               Style);
+  verifyFormat("class Foo {\n"
+         "  int aaaaaaaaaaaaaa(int bbbbbbbbbbbbbb)\n"
+         "  {\n"
+         "    return 0;\n"
+         "  }\n"
+         "};\n",
+               Style);
+
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
+  verifyFormat("int f() {}",
+               Style);
+  verifyFormat("int f() { return 0; }",
+               Style);
+  verifyFormat("int aaaaaaaaaaaaaa(int bbbbbbbbbbbbbb)\n"
+         "{}",
+               Style);
+  verifyFormat("int aaaaaaaaaaaaaa(int bbbbbbbbbbbbbb)\n"
+         "{\n"
+         "  return 0;\n"
+         "}",
+               Style);
 }
 
 TEST_F(FormatTest, UnderstandContextOfRecordTypeKeywords) {
   // Elaborate type variable declarations.
   verifyFormat("struct foo a = {bar};\nint n;");
   verifyFormat("class foo a = {bar};\nint n;");
   verifyFormat("union foo a = {bar};\nint n;");
 
   // Elaborate types inside function definitions.
   verifyFormat("struct foo f() {}\nint n;");
   verifyFormat("class foo f() {}\nint n;");
   verifyFormat("union foo f() {}\nint n;");
 
   // Templates.
   verifyFormat("template <class X> void f() {}\nint n;");
   verifyFormat("template <struct X> void f() {}\nint n;");