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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
