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 ↗

(On Diff #99934)

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 ↗

(On Diff #99934)

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 ↗

(On Diff #99934)

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

6067 ↗

(On Diff #100053)

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

644 ↗

(On Diff #100412)

Reflow the comment.

654 ↗

(On Diff #100412)

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

unittests/Format/FormatTest.cpp

6092 ↗

(On Diff #100412)

indent. Here and elsewhere.

[Typz]

fix indent & rename option to SplitEmptyFunctionBody

[Typz]

ping

[Typz]

ping?

[djasper]

Looks good.

cfe/trunk/include/clang/Format/Format.h

struct BraceWrappingFlags {
     ///   false:
     ///   if (foo()) {
     ///   } else {
     ///   }
     /// \endcode
     bool BeforeElse;
     /// \brief Indent the wrapped braces themselves.
     bool IndentBraces;
+    /// \brief If ``false``, 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. the `AfterFunction` brace wrapping mode is
+    /// set, and the function could/should not be put on a single line (as per
+    /// `AllowShortFunctionsOnASingleLine` and constructor formatting options).
+    /// \code
+    ///   int f()   vs.   inf f()
+    ///   {}              {
+    ///                   }
+    /// \endcode
+    ///
+    bool SplitEmptyFunctionBody;
   };
 
   /// \brief Control of individual brace wrapping cases.
   ///
   /// If ``BreakBeforeBraces`` is set to ``BS_Custom``, use this to specify how
   /// each individual brace case should be handled. Otherwise, this is ignored.
   BraceWrappingFlags BraceWrapping;
 

cfe/trunk/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle::BraceWrappingFlags &Wrapping) {
     IO.mapOptional("AfterFunction", Wrapping.AfterFunction);
     IO.mapOptional("AfterNamespace", Wrapping.AfterNamespace);
     IO.mapOptional("AfterObjCDeclaration", Wrapping.AfterObjCDeclaration);
     IO.mapOptional("AfterStruct", Wrapping.AfterStruct);
     IO.mapOptional("AfterUnion", Wrapping.AfterUnion);
     IO.mapOptional("BeforeCatch", Wrapping.BeforeCatch);
     IO.mapOptional("BeforeElse", Wrapping.BeforeElse);
     IO.mapOptional("IndentBraces", Wrapping.IndentBraces);
+    IO.mapOptional("SplitEmptyFunctionBody", Wrapping.SplitEmptyFunctionBody);
   }
 };
 
 template <> struct MappingTraits<FormatStyle::IncludeCategory> {
   static void mapping(IO &IO, FormatStyle::IncludeCategory &Category) {
     IO.mapOptional("Regex", Category.Regex);
     IO.mapOptional("Priority", Category.Priority);
   }
std::string ParseErrorCategory::message(int EV) const {
   llvm_unreachable("unexpected parse error");
 }
 
 static FormatStyle expandPresets(const FormatStyle &Style) {
   if (Style.BreakBeforeBraces == FormatStyle::BS_Custom)
     return Style;
   FormatStyle Expanded = Style;
   Expanded.BraceWrapping = {false, false, false, false, false, false,
-                            false, false, false, false, false};
+                            false, false, false, false, false, true};
   switch (Style.BreakBeforeBraces) {
   case FormatStyle::BS_Linux:
     Expanded.BraceWrapping.AfterClass = true;
     Expanded.BraceWrapping.AfterFunction = true;
     Expanded.BraceWrapping.AfterNamespace = true;
     break;
   case FormatStyle::BS_Mozilla:
     Expanded.BraceWrapping.AfterClass = true;
     Expanded.BraceWrapping.AfterEnum = true;
     Expanded.BraceWrapping.AfterFunction = true;
     Expanded.BraceWrapping.AfterStruct = true;
     Expanded.BraceWrapping.AfterUnion = true;
+    Expanded.BraceWrapping.SplitEmptyFunctionBody = false;
     break;
   case FormatStyle::BS_Stroustrup:
     Expanded.BraceWrapping.AfterFunction = true;
     Expanded.BraceWrapping.BeforeCatch = true;
     Expanded.BraceWrapping.BeforeElse = true;
     break;
   case FormatStyle::BS_Allman:
     Expanded.BraceWrapping.AfterClass = true;
     Expanded.BraceWrapping.AfterControlStatement = true;
     Expanded.BraceWrapping.AfterEnum = true;
     Expanded.BraceWrapping.AfterFunction = true;
     Expanded.BraceWrapping.AfterNamespace = true;
     Expanded.BraceWrapping.AfterObjCDeclaration = true;
     Expanded.BraceWrapping.AfterStruct = true;
     Expanded.BraceWrapping.BeforeCatch = true;
     Expanded.BraceWrapping.BeforeElse = true;
     break;
   case FormatStyle::BS_GNU:
     Expanded.BraceWrapping = {true, true, true, true, true, true,
-                              true, true, true, true, true};
+                              true, true, true, true, true, true};
     break;
   case FormatStyle::BS_WebKit:
     Expanded.BraceWrapping.AfterFunction = true;
     break;
   default:
     break;
   }
   return Expanded;
FormatStyle getLLVMStyle() {
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AlwaysBreakTemplateDeclarations = false;
   LLVMStyle.BinPackParameters = true;
   LLVMStyle.BinPackArguments = true;
   LLVMStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
   LLVMStyle.BreakBeforeTernaryOperators = true;
   LLVMStyle.BreakBeforeBraces = FormatStyle::BS_Attach;
   LLVMStyle.BraceWrapping = {false, false, false, false, false, false,
-                             false, false, false, false, false};
+                             false, false, false, false, false, true};
   LLVMStyle.BreakAfterJavaFieldAnnotations = false;
   LLVMStyle.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon;
   LLVMStyle.BreakBeforeInheritanceComma = false;
   LLVMStyle.BreakStringLiterals = true;
   LLVMStyle.ColumnLimit = 80;
   LLVMStyle.CommentPragmas = "^ IWYU pragma:";
   LLVMStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = false;
   LLVMStyle.ConstructorInitializerIndentWidth = 4;

cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp

tryFitMultipleLinesInOne(unsigned Indent,
     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;
 
+    if (TheLine->Last->is(TT_FunctionLBrace) &&
+        TheLine->First == TheLine->Last &&
+        !Style.BraceWrapping.SplitEmptyFunctionBody &&
+        I[1]->First->is(tok::r_brace))
+      return tryMergeSimpleBlock(I, E, Limit);
+
     // FIXME: TheLine->Level != 0 might or might not be the right check to do.
     // If necessary, change to something smarter.
     bool MergeShortFunctions =
         Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_All ||
         (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty &&
          I[1]->First->is(tok::r_brace)) ||
         (Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_Inline &&
          TheLine->Level != 0);
if (I[1]->First->is(TT_FunctionLBrace) &&
         return 0;
 
       // Check for Limit <= 2 to account for the " {".
       if (Limit <= 2 || (Style.ColumnLimit == 0 && containsMustBreak(TheLine)))
         return 0;
       Limit -= 2;
 
       unsigned MergedLines = 0;
-      if (MergeShortFunctions) {
+      if (MergeShortFunctions ||
+          (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty &&
+           I[1]->First == I[1]->Last && I + 2 != E &&
+           I[2]->First->is(tok::r_brace))) {
         MergedLines = tryMergeSimpleBlock(I + 1, E, Limit);
         // If we managed to merge the block, count the function header, which is
         // on a separate line.
         if (MergedLines > 0)
           ++MergedLines;
       }
       return MergedLines;
     }

cfe/trunk/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);
+
+  // Also verify behavior when BraceWrapping.AfterFunction = true
+  MergeEmptyOnly.BreakBeforeBraces = FormatStyle::BS_Custom;
+  MergeEmptyOnly.BraceWrapping.AfterFunction = true;
+  verifyFormat("int f() {}", MergeEmptyOnly);
+  verifyFormat("class C {\n"
+               "  int f() {}\n"
+               "};",
+               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);
+
+  // Also verify behavior when BraceWrapping.AfterFunction = true
+  MergeInlineOnly.BreakBeforeBraces = FormatStyle::BS_Custom;
+  MergeInlineOnly.BraceWrapping.AfterFunction = true;
+  verifyFormat("class C {\n"
+               "  int f() { return 42; }\n"
+               "};",
+               MergeInlineOnly);
+  verifyFormat("int f()\n"
+               "{\n"
+               "  return 42;\n"
+               "}",
+               MergeInlineOnly);
+
+  // SFS_Inline implies SFS_Empty
+  verifyFormat("int f() {}", MergeInlineOnly);
+  verifyFormat("class C {\n"
+               "  int f() {}\n"
+               "};",
+               MergeInlineOnly);
+}
+
+TEST_F(FormatTest, SplitEmptyFunctionBody) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterFunction = true;
+  Style.BraceWrapping.SplitEmptyFunctionBody = false;
+  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);
+  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;");
 
TEST_F(FormatTest, ParsesConfigurationBools) {
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterFunction);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterNamespace);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterObjCDeclaration);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterStruct);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterUnion);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, BeforeCatch);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, BeforeElse);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, IndentBraces);
+  CHECK_PARSE_NESTED_BOOL(BraceWrapping, SplitEmptyFunctionBody);
 }
 
 #undef CHECK_PARSE_BOOL
 
 TEST_F(FormatTest, ParsesConfiguration) {
   FormatStyle Style = {};
   Style.Language = FormatStyle::LK_Cpp;
   CHECK_PARSE("AccessModifierOffset: -1234", AccessModifierOffset, -1234);