[PATCH] D132256: [clang-format] Add DefinitionBlockSpacing option

[Alecto Irene Perez via Phabricator via cfe-commits]

alecto created this revision.
alecto added reviewers: Richard, smith.
Herald added a project: All.
alecto requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132256

Files:
  clang/include/clang/Format/Format.h
  clang/lib/Format/DefinitionBlockSeparator.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/DefinitionBlockSeparatorTest.cpp

Index: clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
===
--- clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
+++ clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
@@ -601,6 +601,90 @@
"}",
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
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -746,6 +746,7 @@
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);
@@ -1244,6 +1245,7 @@
   // Off by default Qualifier ordering
   LLVMStyle.QualifierAlignment = FormatStyle::QAS_Leave;
 
+  LLVMStyle.DefinitionBlockSpacing = 1;
   LLVMStyle.DeriveLineEnding = true;
   LLVMStyle.DerivePointerAlignment = false;
   LLVMStyle.EmptyLineAfterAcce

[PATCH] D132256: [clang-format] Add DefinitionBlockSpacing option

[Alecto Irene Perez via Phabricator via cfe-commits]

alecto updated this revision to Diff 454343.
alecto added a comment.

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`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132256/new/

https://reviews.llvm.org/D132256

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/DefinitionBlockSeparator.cpp
  clang/lib/Format/Format.cpp
  clang/unittests/Format/DefinitionBlockSeparatorTest.cpp

Index: clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
===
--- clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
+++ clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
@@ -601,6 +601,90 @@
"}",
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
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -746,6 +746,7 @@
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);
@@ -1244,6 +1245,7 @@
   // Off by default Qualifier ordering
   LLVMStyle.QualifierAlignment = FormatStyle::QAS_Leave;
 
+  LLVMStyle.DefinitionBlockSpacing = 1;
   LLVMStyle.DeriveLineEnding = true;
   LLVMStyle.DerivePointerAlignment = false;
   LLVMStyle.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Never;
Index: clang/lib/Format/DefinitionBlockSeparator.cpp
===
--- clang/lib/Format/DefinitionBlockSeparator.cpp
+++ clang/lib/Format/DefinitionBlockSeparator.cpp
@@ -66,7 +66,9 @@
 return false;
   };
   unsigned NewlineCount =
-  (Style.SeparateDefinitionBlocks == FormatStyle::SDS_Always ? 1 : 0) + 1;
+  1 + (Style.SeparateDefinitionBlocks == FormatStyle::SDS_Always
+   ? Style.DefinitionBlockSpacing
+   : 0);
   WhitespaceManager Whitespaces(

[PATCH] D132256: [clang-format] Add DefinitionBlockSpacing option

[Alecto Irene Perez via Phabricator via cfe-commits]

alecto added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132256/new/

https://reviews.llvm.org/D132256

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132256: [clang-format] Add DefinitionBlockSpacing option

[Alecto Irene Perez via Phabricator via cfe-commits]

alecto added a comment.

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?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132256/new/

https://reviews.llvm.org/D132256

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits