[PATCH] D116314: [clang-format] Add style to separate definition blocks

[Marek Kurdej via Phabricator via cfe-commits]

curdeius requested changes to this revision.
curdeius added a comment.
This revision now requires changes to proceed.

Nice job. Some minor comments.
Please don't forget to reformat too.



================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3415
+
+     SeparateDefinitions
+     Never                v.s.   Always
----------------
Keep it consistent please.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3418
+     #include <cstring>          #include <cstring>
+     struct Foo{
+       int a,b,c;                struct Foo {
----------------
Please reformat the code, e.g. on the left misses spaces (near braces and commas for instance). The difference between left and right should only be blank lines between blocks.
Of course, do it in Format.h and regenerate the RST.


================
Comment at: clang/include/clang/Format/Format.h:3054
+  enum SeparateDefinitionStyle {
+    /// Leave definition blocks separated as they are without changing anything.
+    SDS_Leave,
----------------



================
Comment at: clang/include/clang/Format/Format.h:3063
+  /// Specifies the use of empty lines to separate definition blocks, including
+  /// classes, structs, enum, and functions, for C++ language.
+  /// \code
----------------
Why not C and other similar languages?
Also, you don't seem to be limiting the pass to C++. Please add some tests to other languages.


================
Comment at: clang/include/clang/Format/Format.h:3065
+  /// \code
+  ///    SeparateDefinitions
+  ///    Never                v.s.   Always
----------------
Actuay, do you need this line?


================
Comment at: clang/include/clang/Format/Format.h:4080
+/// Use empty line to separate definition blocks including classes, structs,
+/// functions, namespaces, and enum in the given \p Ranges in \p Code.
+///
----------------



================
Comment at: clang/include/clang/Format/Format.h:4082
+///
+/// Returns the ``Replacements`` that inserts empty lines to separate definition
+/// blocks in all \p Ranges in \p Code.
----------------
?


================
Comment at: clang/unittests/Format/DefinitionBlockSeparatorTest.cpp:48
+  Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
+  EXPECT_EQ("int foo() {\n"
+            "  int i, j;\n"
----------------
Maybe you can add `verifyFormat` helper as in other files? So that you don't need to repeat the code in some cases and we test the code formatting stability as well.


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

https://reviews.llvm.org/D116314