[clang-format] Add DefinitionBlockSpacing option

[alecto]

Summary

Fixes https://github.com/llvm/llvm-project/issues/57180

Given configuration:

SeparateDefinitionBlocks: Always
DefinitionBlockSpacing: 2

Two spaces will be inserted between definition blocks. Code before
formatting:

/// f is a function
void f() {
    // Some code
    // More code
    return;
}
/// g is another function
void g() {
    // Some other code
}

Versus code after formatting:

/// f is a function
void f() {
    // Some code
    // More code
    return;
}
​
​
/// g is another function
void g() {
    // Some other code
}

Note: two zero width spaces were inserted into the commit message in
order to prevent git from removing the additional lines inside the
commit. These will not appear in formatted code.

[HazardyKnusperkeks]

Please run clang/docs/tools/dump_format_style.py.

clang/include/clang/Format/Format.h

3189

This isn't going to land in 15.

3190

Please sort alphabetically.

clang/unittests/Format/DefinitionBlockSeparatorTest.cpp

609

610

Do we want to define the behavior of MaxEmptyLinesToKeep < DefinitionBlockSpacing? Then we need tests for that!
Tests with MaxEmptyLinesToKeep < DefinitionBlockSpacing and initially more than DefinitionBlockSpacing should definitely be there.

[MyDeveloperDay]

nice

[alecto]

Revise based on feedback from Björn Schäpers

We updated the version tag to 16 and positioned the DefinitionBLockSpacing field alphabetically. We also updated the documentation with clang/docs/tools/dump_format_style.py

[alecto]

In D132256#3736076, @HazardyKnusperkeks wrote:

Do we want to define the behavior of MaxEmptyLinesToKeep < DefinitionBlockSpacing? Then we need tests for that!
Tests with MaxEmptyLinesToKeep < DefinitionBlockSpacing and initially more than DefinitionBlockSpacing should definitely be there.

Hello! To clarify, do you mean there should be unit tests for this behavior, or do you mean that there should be checks in clang-format itself that will alert the user if MaxEmptyLinesToKeep < DefinitionBlockSpacing?

Also, is the code in the clang-format examples hand-aligned or is there some tool or script used to align/generate it?

[HazardyKnusperkeks]

In D132256#3738407, @alecto wrote:

In D132256#3736076, @HazardyKnusperkeks wrote:

Do we want to define the behavior of MaxEmptyLinesToKeep < DefinitionBlockSpacing? Then we need tests for that!
Tests with MaxEmptyLinesToKeep < DefinitionBlockSpacing and initially more than DefinitionBlockSpacing should definitely be there.

Hello! To clarify, do you mean there should be unit tests for this behavior, or do you mean that there should be checks in clang-format itself that will alert the user if MaxEmptyLinesToKeep < DefinitionBlockSpacing?

I ask the open question if the behavior should be defined (in howsoever), then there have to be tests, so we can ensure the behavior doesn't regress. Or if it should be undefined, then we don't need tests, because it can do anything.
I don't know if we can actually warn about such things, but we can error out.

I have no opinion on either variant, I just want it to be discussed.

Also, is the code in the clang-format examples hand-aligned or is there some tool or script used to align/generate it?

You can always just write the code and run the tests, and then take a look at the formatted code and copy-paste it, if it is what you want. That's the way I develop. But the code in the strings has to be put somewhat manually there.

[alecto]

If possible I would like to error out if MaxEmptyLinesToKeep < DefinitionBlockSpacing, because that case is probably a mistake on the part of the user, and a failure would be better than clang-format silently doing something unexpected, but I'm not sure where to put the logic for failure.

Where does clang-format handle style/input validation, and what method does it use for handling errors? (Does it rely on exceptions, or returning a "failure" state of some kind?)

[HazardyKnusperkeks]

In D132256#3740714, @alecto wrote:

If possible I would like to error out if MaxEmptyLinesToKeep < DefinitionBlockSpacing, because that case is probably a mistake on the part of the user, and a failure would be better than clang-format silently doing something unexpected, but I'm not sure where to put the logic for failure.

Where does clang-format handle style/input validation, and what method does it use for handling errors? (Does it rely on exceptions, or returning a "failure" state of some kind?)

Take a look at https://github.com/llvm/llvm-project/blob/2b8f722e630d0fdf1ca267361866a27c8d4c9387/clang/lib/Format/Format.cpp#L1696

[MyDeveloperDay]

clang/docs/ClangFormatStyleOptions.rst

2429 ↗

(On Diff #454343)

Can you write this not in terms of the code variables and enumerated but in terms of what they put in the .clang-format file

[MyDeveloperDay]

Cloud you include a test that contains multiple levels of nested scope, I'm assuming we won't add an additonal line at every {} level (or will we?)

clang/include/clang/Format/Format.h

///    NS_TABLE_.*_END$"
   ///                                    };
   ///
   ///                                    class C {};
   ///                                    }
   /// \endcode
   /// \version 14
   SeparateDefinitionStyle SeparateDefinitionBlocks;
 
+  /// The number of empty lines to use when separating definition blocks.
+  /// Defaults to 1.
+  ///
+  /// This determines the number of empty lines to use when
+  /// SeparateDefinitionBlocks == SeparateDefinitionStyle::SDS_Always.
+  ///
+  /// Note: if MaxEmptyLinesToKeep < DefinitionBlockSpacing, then additional
+  /// lines will be removed. MaxEmptyLinesToKeep should be greater than or equal
+  /// to DefinitionBlockSpacing.
+  /// \version 15
+  unsigned DefinitionBlockSpacing;
+
   /// The maximal number of unwrapped lines that a short namespace spans.
   /// Defaults to 1.
   ///
   /// This determines the maximum length of short namespaces by counting
   /// unwrapped lines (i.e. containing neither opening nor closing
   /// namespace brace) and makes "FixNamespaceComments" omit adding
   /// end comments for those.
   /// \code
return AccessModifierOffset == R.AccessModifierOffset &&
            BreakAfterJavaFieldAnnotations == R.BreakAfterJavaFieldAnnotations &&
            BreakStringLiterals == R.BreakStringLiterals &&
            ColumnLimit == R.ColumnLimit && CommentPragmas == R.CommentPragmas &&
            BreakInheritanceList == R.BreakInheritanceList &&
            ConstructorInitializerIndentWidth ==
                R.ConstructorInitializerIndentWidth &&
            ContinuationIndentWidth == R.ContinuationIndentWidth &&
            Cpp11BracedListStyle == R.Cpp11BracedListStyle &&
+           DefinitionBlockSpacing == R.DefinitionBlockSpacing &&
            DeriveLineEnding == R.DeriveLineEnding &&
            DerivePointerAlignment == R.DerivePointerAlignment &&
            DisableFormat == R.DisableFormat &&
            EmptyLineAfterAccessModifier == R.EmptyLineAfterAccessModifier &&
            EmptyLineBeforeAccessModifier == R.EmptyLineBeforeAccessModifier &&
            ExperimentalAutoDetectBinPacking ==
                R.ExperimentalAutoDetectBinPacking &&
            PackConstructorInitializers == R.PackConstructorInitializers &&

clang/lib/Format/DefinitionBlockSeparator.cpp

for (const FormatToken *CurrentToken = Line->First; CurrentToken;
         if (!ExcludeEnum && CurrentToken->is(tok::kw_enum))
           return true;
       }
       BracketLevel += GetBracketLevelChange(CurrentToken);
     }
     return false;
   };
   unsigned NewlineCount =
-      (Style.SeparateDefinitionBlocks == FormatStyle::SDS_Always ? 1 : 0) + 1;
+      1 + (Style.SeparateDefinitionBlocks == FormatStyle::SDS_Always
+               ? Style.DefinitionBlockSpacing
+               : 0);
   WhitespaceManager Whitespaces(
       Env.getSourceManager(), Style,
       Style.DeriveLineEnding
           ? WhitespaceManager::inputUsesCRLF(
                 Env.getSourceManager().getBufferData(Env.getFileID()),
                 Style.UseCRLF)
           : Style.UseCRLF);
   for (unsigned I = 0; I < Lines.size(); ++I) {

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     else if (Style.QualifierAlignment == FormatStyle::QAS_Custom)
       IO.mapOptional("QualifierOrder", Style.QualifierOrder);
 
     IO.mapOptional("CompactNamespaces", Style.CompactNamespaces);
     IO.mapOptional("ConstructorInitializerIndentWidth",
                    Style.ConstructorInitializerIndentWidth);
     IO.mapOptional("ContinuationIndentWidth", Style.ContinuationIndentWidth);
     IO.mapOptional("Cpp11BracedListStyle", Style.Cpp11BracedListStyle);
+    IO.mapOptional("DefinitionBlockSpacing", Style.DefinitionBlockSpacing);
     IO.mapOptional("DeriveLineEnding", Style.DeriveLineEnding);
     IO.mapOptional("DerivePointerAlignment", Style.DerivePointerAlignment);
     IO.mapOptional("DisableFormat", Style.DisableFormat);
     IO.mapOptional("EmptyLineAfterAccessModifier",
                    Style.EmptyLineAfterAccessModifier);
     IO.mapOptional("EmptyLineBeforeAccessModifier",
                    Style.EmptyLineBeforeAccessModifier);
     IO.mapOptional("ExperimentalAutoDetectBinPacking",
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.CompactNamespaces = false;
   LLVMStyle.ConstructorInitializerIndentWidth = 4;
   LLVMStyle.ContinuationIndentWidth = 4;
   LLVMStyle.Cpp11BracedListStyle = true;
 
   // Off by default Qualifier ordering
   LLVMStyle.QualifierAlignment = FormatStyle::QAS_Leave;
 
+  LLVMStyle.DefinitionBlockSpacing = 1;
   LLVMStyle.DeriveLineEnding = true;
   LLVMStyle.DerivePointerAlignment = false;
   LLVMStyle.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Never;
   LLVMStyle.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_LogicalBlock;
   LLVMStyle.ExperimentalAutoDetectBinPacking = false;
   LLVMStyle.PackConstructorInitializers = FormatStyle::PCIS_BinPack;
   LLVMStyle.FixNamespaceComments = true;
   LLVMStyle.ForEachMacros.push_back("foreach");

clang/unittests/Format/DefinitionBlockSeparatorTest.cpp

verifyFormat("export const enum Foo {\n"
                "}\n"
                "\n"
                "export const enum Bar {\n"
                "  D = 1,\n"
                "  E\n"
                "}",
                Style);
 }
+
+TEST_F(DefinitionBlockSeparatorTest, DefinitionBlockSpacing) {
+  FormatStyle Style = getLLVMStyle();
+  Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
+  Style.DefinitionBlockSpacing = 2;
+  /// This has to be set, otherwise the extra lines will be removed
+  Style.MaxEmptyLinesToKeep = 2;
+  verifyFormat("int foo(int i, int j) {\n"
+               "  int r = i + j;\n"
+               "  return r;\n"
+               "}\n"
+               "\n"
+               "\n"
+               "int bar(int j, int k) {\n"
+               "  int r = j + k;\n"
+               "  return r;\n"
+               "}",
+               Style);
+
+  verifyFormat("struct foo {\n"
+               "  int i, j;\n"
+               "};\n"
+               "\n"
+               "\n"
+               "struct bar {\n"
+               "  int j, k;\n"
+               "};",
+               Style);
+
+  verifyFormat("union foo {\n"
+               "  int i, j;\n"
+               "};\n"
+               "\n"
+               "\n"
+               "union bar {\n"
+               "  int j, k;\n"
+               "};",
+               Style);
+
+  verifyFormat("class foo {\n"
+               "  int i, j;\n"
+               "};\n"
+               "\n"
+               "\n"
+               "class bar {\n"
+               "  int j, k;\n"
+               "};",
+               Style);
+
+  verifyFormat("namespace foo {\n"
+               "int i, j;\n"
+               "}\n"
+               "\n"
+               "\n"
+               "namespace bar {\n"
+               "int j, k;\n"
+               "}",
+               Style);
+
+  verifyFormat("enum Foo { FOO, BAR };\n"
+               "\n"
+               "\n"
+               "enum Bar { FOOBAR, BARFOO };\n",
+               Style);
+
+  FormatStyle BreakAfterReturnTypeStyle = Style;
+  BreakAfterReturnTypeStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_All;
+  // Test uppercased long typename
+  verifyFormat("class Foo {\n"
+               "  void\n"
+               "  Bar(int t, int p) {\n"
+               "    int r = t + p;\n"
+               "    return r;\n"
+               "  }\n"
+               "\n"
+               "\n"
+               "  HRESULT\n"
+               "  Foobar(int t, int p) {\n"
+               "    int r = t * p;\n"
+               "    return r;\n"
+               "  }\n"
+               "}\n",
+               BreakAfterReturnTypeStyle);
+}
 } // namespace
 } // namespace format
 } // namespace clang