ClangFormat - Add one space in inline empty function that can be wrapped on a single line.

[Abpostelnicu]

Summary

At Mozilla out coding style tells that short inline member functions that can be wrapped on a single line should have an empty space between the bracelets, like in this example:

class MyClass
{ 
  MyClass() { }
}

The documentation for this behaviour can be found here.
The purpose of this space is to add this option to clang-format through the variable: AddSpaceInEmptyInlineFunction.

[klimek]

Can you point out where that's spelled out in the style guide? I can't find a reference to it (searching for "{}" or "{ }" doesn't provide any hits).
I'm slightly opposed to this, given that so far I haven't heard a convincing argument why this makes any difference to readability.

[Abpostelnicu]

In D37979#879956, @klimek wrote:

Can you point out where that's spelled out in the style guide? I can't find a reference to it (searching for "{}" or "{ }" doesn't provide any hits).
I'm slightly opposed to this, given that so far I haven't heard a convincing argument why this makes any difference to readability.

I see your point, it's not stetted effectively on the coding style page, but if you look in the coding style you will find this:

// Tiny constructors and destructors can be written on a single line.
MyClass() { ... }

Which translates to the fact that you need an empty space in the empty inline functions. Also we have this bug on this matter. I know that this modification should seem a bit strange but this is the way how most of our code is written in the presented situations.

Also doing a grep in our online repository:
https://dxr.mozilla.org/mozilla-central/search?q=%22%7B+%7D&redirect=false

You can see that the incidence of this behaviour is huge. I know the grep produces also noise but still the occurrence is this scenario is huge.

[klimek]

In D37979#879965, @Abpostelnicu wrote:

Also doing a grep in our online repository:
https://dxr.mozilla.org/mozilla-central/search?q=%22%7B+%7D&redirect=false

You can see that the incidence of this behaviour is huge. I know the grep produces also noise but still the occurrence is this scenario is huge.

I also see lots of code that uses {}? Thus, it seems like the code base is not very strict, and going either way enforced with clang-format will be nicer?
https://dxr.mozilla.org/mozilla-central/search?q=%22+%7B%7D&redirect=true

[sylvestre.ledru]

In D37979#895393, @klimek wrote:

I also see lots of code that uses {}? Thus, it seems like the code base is not very strict, and going either way enforced with clang-format will be nicer?
https://dxr.mozilla.org/mozilla-central/search?q=%22+%7B%7D&redirect=true

Indeed, this is why we are working on enforcing that.
This patch is one of the last change to match exactly our coding style. This is why Andi wrote it.

If you could accept it, it would be great. If you won't, please let us know to see what we can do on our side. Thanks!

[djasper]

I believe that this extra option is not worth its cost in terms of maintainability and (more importantly) discoverability of options. You say that the behavior is documented in your coding style, but it is not (probably for the same reason). There isn't even a single example of this, I think. As such, I don't think we should go forward with adding this option.

[klimek]

In D37979#895596, @sylvestre.ledru wrote:

In D37979#895393, @klimek wrote:

I also see lots of code that uses {}? Thus, it seems like the code base is not very strict, and going either way enforced with clang-format will be nicer?
https://dxr.mozilla.org/mozilla-central/search?q=%22+%7B%7D&redirect=true

Indeed, this is why we are working on enforcing that.
This patch is one of the last change to match exactly our coding style. This is why Andi wrote it.

If you could accept it, it would be great. If you won't, please let us know to see what we can do on our side. Thanks!

I'm with Daniel. It looks like there's more code using {} than { } (although I don't know how much noise those searches have), so why not make {} your style rule and enforce that? Style guides are living entities :)

[• mhq199657]

We recently found out the default behaviour of formatting {} changes from {} in clang 8.0 to { } in clang 10.0+. Could someone revive this changeset and kindly review it? We would really like to have a way to specify no space in empty braces.

[sylvestre.ledru]

@mhq199657 we (Mozilla/Firefox) moved to the Google Coding style. So, we are less interested by this change now.

Therefore, we are probably not going to spend time on this as @klimek and @djasper both said no :)

[dfaure-kdab]

In D37979#2447932, @mhq199657 wrote:

We recently found out the default behaviour of formatting {} changes from {} in clang 8.0 to { } in clang 10.0+. Could someone revive this changeset and kindly review it? We would really like to have a way to specify no space in empty braces.

I just experienced the same. If the general agreement is that {} is preferred over { }, why did clang 10.0+ change to { }? Is this a bug? Is this documented anywhere?
Rather than reviving the request for an option that leads to a single space, it seems to me that what we all want is for clang-format to output {} again, right?

docs/ClangFormatStyleOptions.rst

 **AccessModifierOffset** (``int``)
   The extra indent or outdent of access modifiers, e.g. ``public:``.
 
+**AddSpaceInEmptyInlineFunction** (``bool``)
+  if ``true`` empty inline class functions will have an extra space
+  between the two block braces. This is valid only with AfterFunction and
+  SFS_InlineOnly
+
+  .. code-block:: c++
+
+    true:                             false:
+    class T                           class T
+    {                         vs.     {
+      void f() { }                      void f() {}
+    };                                };
+
 **AlignAfterOpenBracket** (``BracketAlignmentStyle``)
   If ``true``, horizontally aligns arguments after an open bracket.
 

docs/ReleaseNotes.rst

   |    #endif            |   #endif             |
   +----------------------+----------------------+
 
+* Option *AddSpaceInEmptyInlineFunction* added to support putting an empty
+  space in between the braces in empty inline class functions. It is used only
+  in combination with AfterFunction and SFS_InlineOnly flags.
+
+  +----------------------+----------------------+
+  | Before               | After                |
+  +======================+======================+
+  |  .. code-block:: c++ | .. code-block:: c++  |
+  |                      |                      |
+  |   class T            |   class T            |
+  |   {                  |   {                  |
+  |     void f() {}      |     void f() { }     |
+  |   };                 |   };                 |
+  +----------------------+----------------------+
+
 * Option -verbose added to the command line.
   Shows the list of processed files.
 

include/clang/Format/Format.h

   /// \endcode
   bool AllowShortCaseLabelsOnASingleLine;
 
+  /// \brief if ``true`` empty inline class functions will have an extra space
+  /// between the two block braces. This is valid only with AfterFunction and
+  /// SFS_InlineOnly
+  /// \code
+  ///   true:                             false:
+  ///   class T                           class T
+  ///   {                         vs.     {
+  ///     void f() { }                      void f() {}
+  ///   };                                };
+  /// \endcode
+  bool AddSpaceInEmptyInlineFunction;
+  
   /// \brief Different styles for merging short functions containing at most one
   /// statement.
   enum ShortFunctionStyle {

            AllowShortBlocksOnASingleLine == R.AllowShortBlocksOnASingleLine &&
            AllowShortCaseLabelsOnASingleLine ==
                R.AllowShortCaseLabelsOnASingleLine &&
+           AddSpaceInEmptyInlineFunction == R.AddSpaceInEmptyInlineFunction &&
            AllowShortFunctionsOnASingleLine ==
                R.AllowShortFunctionsOnASingleLine &&
            AllowShortIfStatementsOnASingleLine ==

lib/Format/Format.cpp

                    Style.AllowShortBlocksOnASingleLine);
     IO.mapOptional("AllowShortCaseLabelsOnASingleLine",
                    Style.AllowShortCaseLabelsOnASingleLine);
+    IO.mapOptional("AddSpaceInEmptyInlineFunction",
+                   Style.AddSpaceInEmptyInlineFunction);
     IO.mapOptional("AllowShortFunctionsOnASingleLine",
                    Style.AllowShortFunctionsOnASingleLine);
     IO.mapOptional("AllowShortIfStatementsOnASingleLine",

   LLVMStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
   LLVMStyle.AllowShortBlocksOnASingleLine = false;
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
+  LLVMStyle.AddSpaceInEmptyInlineFunction = false;
   LLVMStyle.AllowShortIfStatementsOnASingleLine = false;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;

       FormatStyle::RTBS_TopLevel;
   MozillaStyle.AlwaysBreakAfterDefinitionReturnType =
       FormatStyle::DRTBS_TopLevel;
+  MozillaStyle.AddSpaceInEmptyInlineFunction = true;
   MozillaStyle.AlwaysBreakTemplateDeclarations = true;
   MozillaStyle.BinPackParameters = false;
   MozillaStyle.BinPackArguments = false;

lib/Format/UnwrappedLineFormatter.cpp

       Limit -= 2;
 
       unsigned MergedLines = 0;
+      bool EmptyFunc = I[1]->First == I[1]->Last && I + 2 != E &&
+                       I[2]->First->is(tok::r_brace);
       if (MergeShortFunctions ||
           (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty &&
-           I[1]->First == I[1]->Last && I + 2 != E &&
-           I[2]->First->is(tok::r_brace))) {
+           EmptyFunc)) {
         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;
       }
+
+      if (Style.AddSpaceInEmptyInlineFunction && EmptyFunc &&
+          Style.AllowShortFunctionsOnASingleLine &
+              FormatStyle::SFS_InlineOnly &&
+          TheLine->Level != 0) {
+        I[2]->First->SpacesRequiredBefore = 1;
+      }
       return MergedLines;
     }
     if (TheLine->First->is(tok::kw_if)) {

unittests/Format/FormatTest.cpp

                "  int i = 0;\n"
                "}");
 }
+  
+TEST_F(FormatTest, SpaceInEmptyInlineFunction) {
+  FormatStyle Style = getMozillaStyle();
+  Style.AddSpaceInEmptyInlineFunction = true;
+  verifyFormat("class T\n"
+               "{\n"
+               "  void f() { }\n"
+               "};",
+               Style);
+}
 
 //===----------------------------------------------------------------------===//
 // Tests for classes, namespaces, etc.

   CHECK_PARSE_BOOL(AllowAllParametersOfDeclarationOnNextLine);
   CHECK_PARSE_BOOL(AllowShortBlocksOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
+  CHECK_PARSE_BOOL(AddSpaceInEmptyInlineFunction);
   CHECK_PARSE_BOOL(AllowShortIfStatementsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
   CHECK_PARSE_BOOL(AlwaysBreakTemplateDeclarations);