[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Björn Schäpers via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG60bf5826cfd3: [clang-format] PR16518 Add flag to suppress 
empty line insertion before access… (authored by thezbyg, committed by 
HazardyKnusperkeks).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93846

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/test/Format/access-modifiers.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8887,6 +8887,298 @@
getLLVMStyle());
 }
 
+TEST_F(FormatTest, FormatsAccessModifiers) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.EmptyLineBeforeAccessModifier,
+FormatStyle::ELBAMS_LogicalBlock);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo { /* comment */\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Never;
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo { /* comment */\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo { /* comment */\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Always;
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+ 

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Albertas Vyšniauskas via Phabricator via cfe-commits]

thezbyg marked an inline comment as done.
thezbyg added a comment.

Yes, I do not have commit access and would like for someone to commit/push this 
change. My name and email is `Albertas Vyšniauskas `.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Albertas Vyšniauskas via Phabricator via cfe-commits]

thezbyg updated this revision to Diff 319057.
thezbyg added a comment.

Remove redundant line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93846

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/test/Format/access-modifiers.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8887,6 +8887,298 @@
getLLVMStyle());
 }
 
+TEST_F(FormatTest, FormatsAccessModifiers) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.EmptyLineBeforeAccessModifier,
+FormatStyle::ELBAMS_LogicalBlock);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo { /* comment */\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Never;
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo { /* comment */\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo { /* comment */\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Always;
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+  

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:8943
+   Style);
+  Style = getLLVMStyle();
+  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Never;

Redundant line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

Do you need someone to push this? If so please post email and name to use for 
the commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Albertas Vyšniauskas via Phabricator via cfe-commits]

thezbyg updated this revision to Diff 316660.
thezbyg added a comment.

Improved default style tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93846

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/test/Format/access-modifiers.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8887,6 +8887,299 @@
getLLVMStyle());
 }
 
+TEST_F(FormatTest, FormatsAccessModifiers) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.EmptyLineBeforeAccessModifier,
+FormatStyle::ELBAMS_LogicalBlock);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo { /* comment */\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  Style = getLLVMStyle();
+  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Never;
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo { /* comment */\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo { /* comment */\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Always;
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.

One last nit, otherwise LGTM.




Comment at: clang/unittests/Format/FormatTest.cpp:8891
+TEST_F(FormatTest, FormatsAccessModifiers) {
+  verifyFormat("struct foo {\n"
+   "private:\n"

For the ease of understanding that you test `LogicalBlock` in this first part, 
I'd add:
```
  FormatStyle Style = getLLVMStyle();
  EXPECT_EQ(Style.EmptyLineBeforeAccessModifier, 
FormatStyle::ELBAMS_LogicalBlock);
```
and use this `Style` in `verifyFormat`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Albertas Vyšniauskas via Phabricator via cfe-commits]

thezbyg updated this revision to Diff 316511.
thezbyg added a comment.

Renamed DontModify enum to Leave.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93846

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/test/Format/access-modifiers.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8887,6 +8887,292 @@
getLLVMStyle());
 }
 
+TEST_F(FormatTest, FormatsAccessModifiers) {
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n");
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n");
+  verifyFormat("struct foo { /* comment */\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n");
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n");
+  FormatStyle Style = getLLVMStyle();
+  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Never;
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo { /* comment */\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo { /* comment */\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Always;
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

Apart from that last naming issue looks good to me.




Comment at: clang/lib/Format/Format.cpp:235
+IO.enumCase(Value, "Never", FormatStyle::ELBAMS_Never);
+IO.enumCase(Value, "DontModify", FormatStyle::ELBAMS_DontModify);
+IO.enumCase(Value, "LogicalBlock", FormatStyle::ELBAMS_LogicalBlock);

MyDeveloperDay wrote:
> Other places use `Leave` I assume that means the same as DontModify?
If that's the case, we should stick with it. Although I personally find 
DontModify better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/Format.cpp:235
+IO.enumCase(Value, "Never", FormatStyle::ELBAMS_Never);
+IO.enumCase(Value, "DontModify", FormatStyle::ELBAMS_DontModify);
+IO.enumCase(Value, "LogicalBlock", FormatStyle::ELBAMS_LogicalBlock);

Other places use `Leave` I assume that means the same as DontModify?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Albertas Vyšniauskas via Phabricator via cfe-commits]

thezbyg updated this revision to Diff 316446.
thezbyg added a comment.

Rebased changes on master.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93846

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/test/Format/access-modifiers.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8887,6 +8887,292 @@
getLLVMStyle());
 }
 
+TEST_F(FormatTest, FormatsAccessModifiers) {
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n");
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n");
+  verifyFormat("struct foo { /* comment */\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n");
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n");
+  FormatStyle Style = getLLVMStyle();
+  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Never;
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo { /* comment */\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo { /* comment */\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Always;
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+  

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D93846#2488469 , @thezbyg wrote:

> Diff updated. Previous diff was generated after rebase, and Phabricator 
> change preview did not show any unrelated changes, so I thought that 
> everything is fine.

Now your diff removes the stuff?
You should rebase your change on master or main, (ideally) have only one commit 
on top of it, and than diff `HEAD^1`.


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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Albertas Vyšniauskas via Phabricator via cfe-commits]

thezbyg updated this revision to Diff 315578.
thezbyg added a comment.

Diff updated. Previous diff was generated after rebase, and Phabricator change 
preview did not show any unrelated changes, so I thought that everything is 
fine.


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

https://reviews.llvm.org/D93846

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/test/Format/access-modifiers.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8540,6 +8540,292 @@
getLLVMStyle());
 }
 
+TEST_F(FormatTest, FormatsAccessModifiers) {
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n");
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n");
+  verifyFormat("struct foo { /* comment */\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n");
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n");
+  FormatStyle Style = getLLVMStyle();
+  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Never;
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo { /* comment */\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo { /* comment */\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Always;
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "private:\n"
+ 

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

I think your base for the diff is wrong, there are many "added" things which 
are already in clang-format. Could you update the diff?


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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Albertas Vyšniauskas via Phabricator via cfe-commits]

thezbyg updated this revision to Diff 314962.
thezbyg added a comment.

Switched `EmptyLineBeforeAccessModifierStyle` option from bool to enum.
`EmptyLineBeforeAccessModifierStyle` option can now have one of four values: 
`Never`, `DontModify`, `LogicalBlock`, `Always`.
`Never` removes all empty lines before access modifiers.
`DontModify` keeps existing empty lines.
`LogicalBlock` adds empty line only when access modifier starts a new logical 
block (current clang-format behavior).
`Always` adds empty line before all access modifiers except when access 
modifier is at the start of class/struct definition.

Updated tests to test all four option values. When testing `DontModify` option, 
some of the tests use `EXPECT_EQ()` instead of `verifyFormat()`. This is 
because empty lines are not preserved in `verifyFormat()` due to 
`test::messUp()`.


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

https://reviews.llvm.org/D93846

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/test/Format/access-modifiers.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8887,6 +8887,292 @@
getLLVMStyle());
 }
 
+TEST_F(FormatTest, FormatsAccessModifiers) {
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n");
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n");
+  verifyFormat("struct foo { /* comment */\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n");
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n");
+  FormatStyle Style = getLLVMStyle();
+  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Never;
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "private:\n"
+   "  int i;\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo { /* comment */\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo { /* comment */\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "  // comment\n"
+   "\n"
+   "private:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "#ifdef FOO\n"
+   "\n"
+   "private:\n"
+   "#endif\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Always;
+  verifyFormat("struct foo {\n"

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1221
+if (Style.EmptyLineBeforeAccessModifier &&
+PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
+RootToken.NewlinesBefore == 1)

thezbyg wrote:
> HazardyKnusperkeks wrote:
> > thezbyg wrote:
> > > MyDeveloperDay wrote:
> > > > maybe I don't understand the logic but why is this `r_brace` and 
> > > > shouldn't we be looking at the "Last Non Comment" token?
> > > I think that the logic is to only add empty line when access modifier is 
> > > after member function body (`r_brace` indicates this) or declaration of 
> > > field/function. If this check is changed to look at "last non comment" 
> > > token, then empty line will be inserted in code like this:
> > > ```
> > > struct Foo {
> > >   int i;
> > >   //comment
> > > private:
> > >   int j;
> > > }
> > > ```
> > But with the given Name it should add an empty line there! Otherwise you 
> > should use an enum and not a bool to control wether a comment should 
> > suppress the empty line or not. Also you should make the exception(s) clear 
> > with unit tests.
> Current clang-format behavior of access modifier separation was added in 
> commit (fd433365e08da024ddc99dd0d7bce8e89d8c5469) and the test function is 
> called //SeparatesLogicalBlocks//. So could we simply rename this new option 
> to //SeparateLogicalBlocks// and leave the bool type? Option description 
> would contain code examples and further explanation what logical block means. 
> Setting //SeparateLogicalBlocks// option to false would disable empty line 
> insertion, but would not remove existing empty lines.
> 
> Is option name //EmptyLineBeforeAccessModifier// acceptable if enum is used? 
> And how should I name enum values? One enum value could be called //Never//, 
> but other must indicate that empty line is only inserted when access modifier 
> is after member function body or field/function/struct declaration.
> 
> I think that you incorrectly assumed from my previous comment, that only 
> comments suppress empty line insertion. No empty line is inserted in all 
> following code examples:
> ```
> struct Foo {
> private:
>   int j;
> };
> ```
> ```
> struct Foo {
> private:
> public:
>   int j;
> };
> ```
> ```
> struct Foo {
> #ifdef FOO
> private:
> #endif
>   int j;
> };
> ```
> Current clang-format behavior of access modifier separation was added in 
> commit (fd433365e08da024ddc99dd0d7bce8e89d8c5469) and the test function is 
> called //SeparatesLogicalBlocks//. So could we simply rename this new option 
> to //SeparateLogicalBlocks// and leave the bool type? Option description 
> would contain code examples and further explanation what logical block means. 
> Setting //SeparateLogicalBlocks// option to false would disable empty line 
> insertion, but would not remove existing empty lines.
> 
> Is option name //EmptyLineBeforeAccessModifier// acceptable if enum is used? 
> And how should I name enum values? One enum value could be called //Never//, 
> but other must indicate that empty line is only inserted when access modifier 
> is after member function body or field/function/struct declaration.

That now depends on what you want (at least for me), if the name stays 
`EmptyLineBeforeAccessModifier` and it stays a `bool` it should nearly always 
add a space (most likely not directly after the `{`. I think the name would 
still fit for an enum with values like `Always`, `Never`, `DontModify`, and 
similar.

If you change the name it can stay a `bool`, but then needs an explanation with 
examples what the option really means.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1222-1223
+PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
+RootToken.NewlinesBefore == 1)
+  ++Newlines;
+else if (!Style.EmptyLineBeforeAccessModifier &&

thezbyg wrote:
> MyDeveloperDay wrote:
> > HazardyKnusperkeks wrote:
> > > A tab?
> > My experience is that this doesn't mean a tab but just the what phabricator 
> > shows a change in whitespace
> > 
> > it is now 2 levels  of `if` scope indented not 1 so I think it should be 
> > moved over abit
> > 
> > 
> > 
> > 
> Yes, there is no tab in submitted patch, only 6 spaces.
> 
> Is this indented incorrectly?
> ```
> if (PreviousLine && RootToken.isAccessSpecifier()) {
>   if (Style.EmptyLineBeforeAccessModifier &&
>   PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
>   RootToken.NewlinesBefore == 1)
> ++Newlines;
>   else if (!Style.EmptyLineBeforeAccessModifier &&
>RootToken.NewlinesBefore > 1)
> Newlines = 1;
> }
> ```
> I always run `git clang-format-11 HEAD~1` before generating patch file and 
> uploading it here.
I would assume so. I was just irritated by the >>. Everything is fine here.


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

https://reviews.llvm.org/D93846

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Albertas Vyšniauskas via Phabricator via cfe-commits]

thezbyg marked an inline comment as done.
thezbyg added inline comments.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1221
+if (Style.EmptyLineBeforeAccessModifier &&
+PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
+RootToken.NewlinesBefore == 1)

HazardyKnusperkeks wrote:
> thezbyg wrote:
> > MyDeveloperDay wrote:
> > > maybe I don't understand the logic but why is this `r_brace` and 
> > > shouldn't we be looking at the "Last Non Comment" token?
> > I think that the logic is to only add empty line when access modifier is 
> > after member function body (`r_brace` indicates this) or declaration of 
> > field/function. If this check is changed to look at "last non comment" 
> > token, then empty line will be inserted in code like this:
> > ```
> > struct Foo {
> >   int i;
> >   //comment
> > private:
> >   int j;
> > }
> > ```
> But with the given Name it should add an empty line there! Otherwise you 
> should use an enum and not a bool to control wether a comment should suppress 
> the empty line or not. Also you should make the exception(s) clear with unit 
> tests.
Current clang-format behavior of access modifier separation was added in commit 
(fd433365e08da024ddc99dd0d7bce8e89d8c5469) and the test function is called 
//SeparatesLogicalBlocks//. So could we simply rename this new option to 
//SeparateLogicalBlocks// and leave the bool type? Option description would 
contain code examples and further explanation what logical block means. Setting 
//SeparateLogicalBlocks// option to false would disable empty line insertion, 
but would not remove existing empty lines.

Is option name //EmptyLineBeforeAccessModifier// acceptable if enum is used? 
And how should I name enum values? One enum value could be called //Never//, 
but other must indicate that empty line is only inserted when access modifier 
is after member function body or field/function/struct declaration.

I think that you incorrectly assumed from my previous comment, that only 
comments suppress empty line insertion. No empty line is inserted in all 
following code examples:
```
struct Foo {
private:
  int j;
};
```
```
struct Foo {
private:
public:
  int j;
};
```
```
struct Foo {
#ifdef FOO
private:
#endif
  int j;
};
```



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1222-1223
+PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
+RootToken.NewlinesBefore == 1)
+  ++Newlines;
+else if (!Style.EmptyLineBeforeAccessModifier &&

MyDeveloperDay wrote:
> HazardyKnusperkeks wrote:
> > A tab?
> My experience is that this doesn't mean a tab but just the what phabricator 
> shows a change in whitespace
> 
> it is now 2 levels  of `if` scope indented not 1 so I think it should be 
> moved over abit
> 
> 
> 
> 
Yes, there is no tab in submitted patch, only 6 spaces.

Is this indented incorrectly?
```
if (PreviousLine && RootToken.isAccessSpecifier()) {
  if (Style.EmptyLineBeforeAccessModifier &&
  PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
  RootToken.NewlinesBefore == 1)
++Newlines;
  else if (!Style.EmptyLineBeforeAccessModifier &&
   RootToken.NewlinesBefore > 1)
Newlines = 1;
}
```
I always run `git clang-format-11 HEAD~1` before generating patch file and 
uploading it here.



Comment at: clang/unittests/Format/FormatTest.cpp:8612-8625
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",

HazardyKnusperkeks wrote:
> Just curios, any reason why the struct is repeated? I don't seem to notice a 
> difference.
> 
> And by the way, you are missing the `;` at the end of the definition, I don't 
> know if that affects clang-format.
Some of the tests have equal expected and code values. I will update them to 
single parameter `verifyFormat()`.

clang-format does not seem to care about missing `;`, but I will add them as 
all other tests have them.


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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1222-1223
+PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
+RootToken.NewlinesBefore == 1)
+  ++Newlines;
+else if (!Style.EmptyLineBeforeAccessModifier &&

HazardyKnusperkeks wrote:
> A tab?
My experience is that this doesn't mean a tab but just the what phabricator 
shows a change in whitespace

it is now 2 levels  of `if` scope indented not 1 so I think it should be moved 
over abit






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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1221
+if (Style.EmptyLineBeforeAccessModifier &&
+PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
+RootToken.NewlinesBefore == 1)

thezbyg wrote:
> MyDeveloperDay wrote:
> > maybe I don't understand the logic but why is this `r_brace` and shouldn't 
> > we be looking at the "Last Non Comment" token?
> I think that the logic is to only add empty line when access modifier is 
> after member function body (`r_brace` indicates this) or declaration of 
> field/function. If this check is changed to look at "last non comment" token, 
> then empty line will be inserted in code like this:
> ```
> struct Foo {
>   int i;
>   //comment
> private:
>   int j;
> }
> ```
But with the given Name it should add an empty line there! Otherwise you should 
use an enum and not a bool to control wether a comment should suppress the 
empty line or not. Also you should make the exception(s) clear with unit tests.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1222-1223
+PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
+RootToken.NewlinesBefore == 1)
+  ++Newlines;
+else if (!Style.EmptyLineBeforeAccessModifier &&

A tab?



Comment at: clang/unittests/Format/FormatTest.cpp:8612-8625
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",

Just curios, any reason why the struct is repeated? I don't seem to notice a 
difference.

And by the way, you are missing the `;` at the end of the definition, I don't 
know if that affects clang-format.


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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Albertas Vyšniauskas via Phabricator via cfe-commits]

thezbyg marked an inline comment as done.
thezbyg added inline comments.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1221
+if (Style.EmptyLineBeforeAccessModifier &&
+PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
+RootToken.NewlinesBefore == 1)

MyDeveloperDay wrote:
> maybe I don't understand the logic but why is this `r_brace` and shouldn't we 
> be looking at the "Last Non Comment" token?
I think that the logic is to only add empty line when access modifier is after 
member function body (`r_brace` indicates this) or declaration of 
field/function. If this check is changed to look at "last non comment" token, 
then empty line will be inserted in code like this:
```
struct Foo {
  int i;
  //comment
private:
  int j;
}
```


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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1221
+if (Style.EmptyLineBeforeAccessModifier &&
+PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
+RootToken.NewlinesBefore == 1)

maybe I don't understand the logic but why is this `r_brace` and shouldn't we 
be looking at the "Last Non Comment" token?


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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Albertas Vyšniauskas via Phabricator via cfe-commits]

thezbyg added a comment.

When access modifier is in the same line with previous tokens, 
**UnwrappedLineFormatter::formatFirstToken** is called with 
**RootToken.NewlinesBefore** == 0, but empty line is only inserted if 
**RootToken.NewlinesBefore** == 1. The following change fixes this and passes 
all tests except the ones with comment:

 if (PreviousLine && RootToken.isAccessSpecifier()) {
   if (Style.EmptyLineBeforeAccessModifier &&
   PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
  -RootToken.NewlinesBefore == 1)
  -  ++Newlines;
  +RootToken.NewlinesBefore <= 1)
  +  Newlines = 2;
   else if (!Style.EmptyLineBeforeAccessModifier &&
RootToken.NewlinesBefore > 1)
 Newlines = 1;
 }


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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Albertas Vyšniauskas via Phabricator via cfe-commits]

thezbyg marked 2 inline comments as done.
thezbyg added a comment.

After some updating, rebuilding and searching for differences in Objective-C++ 
formatting, I managed to find where the problem with these failing tests is. In 
**_verifyFormat** function C++ code formatting is tested for stability like 
this:

  EXPECT_EQ(Expected.str(), format(Expected, Style));
  EXPECT_EQ(Expected.str(), format(Code, Style));

, but Objective-C++ test, in the same function, looks like this:

  EXPECT_EQ(Expected.str(), format(test::messUp(Code), ObjCStyle));

**test::messUp** function removes all newlines and indentation, so test code:

  struct foo {
int i;
  
  private:
int j;
  }

turns into:

  struct foo { int i; private: int j; }

After running **format** on this code, we get incorrect result:

  struct foo {
int i;
  private:
int j;
  }

Running **format** again would produce the correct output:

  struct foo {
int i;
  
  private:
int j;
  }

So it seems that insertion of empty line fails when access modifier is in the 
same line as previous tokens. Unmodified clang-format produces the same output. 
As this behavior is not related to the changes in my patch, should we attempt 
to fix it here, or a separate bug report would be preferred?

The situation with tests containing comments is similar, because 
**test::messUp** single line output is formatted into:

  struct foo { /* comment */
  private:
int i;
int j;
  }

which is not the same as:

  struct foo {
/* comment */
  private:
int i;
int j;
  }

In my opinion, Objective-C++ **test::messUp** test incorrectly expects any code 
collapsed into a single line to format into the same code as formatted original 
code.


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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:8544
+TEST_F(FormatTest, FormatsAccessModifiers) {
+  EXPECT_EQ("struct foo {\n"
+"  int i;\n"

any reason this can't be verifyFormat?



Comment at: clang/unittests/Format/FormatTest.cpp:8565
+   "}\n");
+  EXPECT_EQ("struct foo {\n"
+"  int i;\n"

verifyFormat? why not


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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

ok so this looks ok, but before we commit can we have a discussion about why 
you think it fails for the comment case?


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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Albertas Vyšniauskas via Phabricator via cfe-commits]

thezbyg updated this revision to Diff 314129.
thezbyg marked 3 inline comments as done.
thezbyg added a comment.

Added missing full stop.
Executed clang/doc/tools/dump_style.py to update ClangFormatStyleOptions.rst.


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

https://reviews.llvm.org/D93846

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/test/Format/access-modifiers.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8540,6 +8540,186 @@
getLLVMStyle());
 }
 
+TEST_F(FormatTest, FormatsAccessModifiers) {
+  EXPECT_EQ("struct foo {\n"
+"  int i;\n"
+"\n"
+"private:\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"
+   "  int i;\n"
+   "\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n",
+   getLLVMStyle()));
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  int i;\n"
+   "}\n",
+   "struct foo {\n"
+   "private:\n"
+   "  int i;\n"
+   "}\n");
+  EXPECT_EQ("struct foo {\n"
+"  int i;\n"
+"\n"
+"private:\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"
+   "  int i;\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n",
+   getLLVMStyle()));
+  verifyFormat("struct foo {\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n",
+   "struct foo {\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n");
+  EXPECT_EQ("struct foo {\n"
+"  /* comment */\n"
+"private:\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"
+   "  /* comment */\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n",
+   getLLVMStyle()));
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   "struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n");
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   "struct foo {\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n");
+  FormatStyle Style = getLLVMStyle();
+  Style.EmptyLineBeforeAccessModifier = false;
+  verifyFormat("struct foo {\n"
+   "  int i;\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n",
+   "struct foo {\n"
+   "  int i;\n"
+   "\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   "struct foo {\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   "struct foo {\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "  // comment\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   "struct foo {\n"
+   "  // comment\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   Style);
+  EXPECT_EQ("struct foo {\n"
+"  /* comment */\n"
+"private:\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"
+   "  /* comment */\n"
+   "\n"
+   

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/include/clang/Format/Format.h:1351
 
+  /// If true, the empty line is inserted before access modifiers
+  /// \code

The full stop will go here then regenerate.


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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

Nit.




Comment at: clang/docs/ClangFormatStyleOptions.rst:1545
+**EmptyLineBeforeAccessModifier** (``bool``)
+  If true, the empty line is inserted before access modifiers
+

Full stop at the end of the phrase.


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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Albertas Vyšniauskas via Phabricator via cfe-commits]

thezbyg updated this revision to Diff 313973.
thezbyg added a comment.

Switched to verifyFormat in most of the tests.
Rerun clang-format.


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

https://reviews.llvm.org/D93846

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/test/Format/access-modifiers.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8540,6 +8540,186 @@
getLLVMStyle());
 }
 
+TEST_F(FormatTest, FormatsAccessModifiers) {
+  EXPECT_EQ("struct foo {\n"
+"  int i;\n"
+"\n"
+"private:\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"
+   "  int i;\n"
+   "\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n",
+   getLLVMStyle()));
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  int i;\n"
+   "}\n",
+   "struct foo {\n"
+   "private:\n"
+   "  int i;\n"
+   "}\n");
+  EXPECT_EQ("struct foo {\n"
+"  int i;\n"
+"\n"
+"private:\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"
+   "  int i;\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n",
+   getLLVMStyle()));
+  verifyFormat("struct foo {\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n",
+   "struct foo {\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n");
+  EXPECT_EQ("struct foo {\n"
+"  /* comment */\n"
+"private:\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"
+   "  /* comment */\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n",
+   getLLVMStyle()));
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   "struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n");
+  verifyFormat("struct foo {\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   "struct foo {\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n");
+  FormatStyle Style = getLLVMStyle();
+  Style.EmptyLineBeforeAccessModifier = false;
+  verifyFormat("struct foo {\n"
+   "  int i;\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n",
+   "struct foo {\n"
+   "  int i;\n"
+   "\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   "struct foo {\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   "struct foo {\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   Style);
+  verifyFormat("struct foo {\n"
+   "  // comment\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   "struct foo {\n"
+   "  // comment\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   Style);
+  EXPECT_EQ("struct foo {\n"
+"  /* comment */\n"
+"private:\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"
+   "  /* comment */\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:8544-8556
+  EXPECT_EQ("struct foo {\n"
+"  int i;\n"
+"\n"
+"private:\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"

thezbyg wrote:
> MyDeveloperDay wrote:
> > if you use verifyFormat it will check what happens when it messes the code 
> > up to ensure its stable
> After switching to verifyFormat all tests pass when checking C++ formatting, 
> but some of the same tests fail in Objective-C++ check:
> C style comment is attached to previous line:
> ```
>   Expected: Expected.str()
>   Which is: "struct foo {\n  /* comment */\nprivate:\n  int i;\n  int 
> j;\n}\n"
> To be equal to: format(test::messUp(Code), ObjCStyle)
>   Which is: "struct foo { /* comment */\nprivate:\n  int i;\n  int 
> j;\n}\n"
> With diff:
> @@ -1,4 +1,3 @@
> -struct foo {
> -  /* comment */
> +struct foo { /* comment */
>  private:
>int i;
> 
> ```
> Empty line before access modifier is removed:
> ```
>   Expected: Expected.str()
>   Which is: "struct foo {\n  int i;\n\nprivate:\n  int j;\n}\n"
> To be equal to: format(test::messUp(Code), ObjCStyle)
>   Which is: "struct foo {\n  int i;\nprivate:\n  int j;\n}\n"
> With diff:
> @@ -1,5 @@
>  struct foo {
>int i;
> -
>  private:
>int j;
> ```
> Looks like empty lines before modifiers are removed for Objective-C++ 
> language. What should I do?
please update the patch at least to user verifyFomat where you can.

Its likely means we are missing something


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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Albertas Vyšniauskas via Phabricator via cfe-commits]

thezbyg added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:8544-8556
+  EXPECT_EQ("struct foo {\n"
+"  int i;\n"
+"\n"
+"private:\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"

MyDeveloperDay wrote:
> if you use verifyFormat it will check what happens when it messes the code up 
> to ensure its stable
After switching to verifyFormat all tests pass when checking C++ formatting, 
but some of the same tests fail in Objective-C++ check:
C style comment is attached to previous line:
```
  Expected: Expected.str()
  Which is: "struct foo {\n  /* comment */\nprivate:\n  int i;\n  int 
j;\n}\n"
To be equal to: format(test::messUp(Code), ObjCStyle)
  Which is: "struct foo { /* comment */\nprivate:\n  int i;\n  int j;\n}\n"
With diff:
@@ -1,4 +1,3 @@
-struct foo {
-  /* comment */
+struct foo { /* comment */
 private:
   int i;

```
Empty line before access modifier is removed:
```
  Expected: Expected.str()
  Which is: "struct foo {\n  int i;\n\nprivate:\n  int j;\n}\n"
To be equal to: format(test::messUp(Code), ObjCStyle)
  Which is: "struct foo {\n  int i;\nprivate:\n  int j;\n}\n"
With diff:
@@ -1,5 @@
 struct foo {
   int i;
-
 private:
   int j;
```
Looks like empty lines before modifiers are removed for Objective-C++ language. 
What should I do?


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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Albertas Vyšniauskas via Phabricator via cfe-commits]

thezbyg marked 10 inline comments as done.
thezbyg added inline comments.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1220
+  if (Style.InsertEmptyLineBeforeAccessModifier && PreviousLine &&
+  PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
   RootToken.isAccessSpecifier() && RootToken.NewlinesBefore == 1)

curdeius wrote:
> Just thinking out loud, but shouldn't we just check that 
> `PreviousLine->Last->isNot(tok::l_brace)`?
> Could you please add a test with comments before access modifiers?
No previous line token check is necessary if it is acceptable to remove empty 
lines in situations where EmptyLineBeforeAccessModifier=true would not add 
empty line.

Without any additional tests formatting the following code with 
EmptyLineBeforeAccessModifier=true:

```
struct foo {
private:
  int i;
}
```
would not add any empty lines, but formatting this code with 
EmptyLineBeforeAccessModifier=false:

```
struct foo {

private:
  int i;
}
```
would remove empty line before modifier.


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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:8544-8556
+  EXPECT_EQ("struct foo {\n"
+"  int i;\n"
+"\n"
+"private:\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"

if you use verifyFormat it will check what happens when it messes the code up 
to ensure its stable


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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Albertas Vyšniauskas via Phabricator via cfe-commits]

thezbyg updated this revision to Diff 313863.
thezbyg added a comment.

Option renamed to EmptyLineBeforeAccessModifier.
Placed new configuration member in correct place alphabetically.
Last token in previous line is no longer checked when 
EmptyLineBeforeAccessModifier is false.
Executed clang/doc/tools/dump_style.py to update ClangFormatsStyleOptions.rst.
Unit tests added to clang/unittests/Format/FormatTests.cpp.


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

https://reviews.llvm.org/D93846

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/test/Format/access-modifiers.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8540,6 +8540,190 @@
getLLVMStyle());
 }
 
+TEST_F(FormatTest, FormatsAccessModifiers) {
+  EXPECT_EQ("struct foo {\n"
+"  int i;\n"
+"\n"
+"private:\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"
+   "  int i;\n"
+   "\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n",
+   getLLVMStyle()));
+  EXPECT_EQ("struct foo {\n"
+"private:\n"
+"  int i;\n"
+"}\n",
+format("struct foo {\n"
+   "private:\n"
+   "  int i;\n"
+   "}\n",
+   getLLVMStyle()));
+  EXPECT_EQ("struct foo {\n"
+"  int i;\n"
+"\n"
+"private:\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"
+   "  int i;\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n",
+   getLLVMStyle()));
+  EXPECT_EQ("struct foo {\n"
+"  // comment\n"
+"private:\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"
+   "  // comment\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n",
+   getLLVMStyle()));
+  EXPECT_EQ("struct foo {\n"
+"  /* comment */\n"
+"private:\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"
+   "  /* comment */\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n",
+   getLLVMStyle()));
+  EXPECT_EQ("struct foo {\n"
+"#ifdef FOO\n"
+"#endif\n"
+"private:\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"
+   "#ifdef FOO\n"
+   "#endif\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   getLLVMStyle()));
+  EXPECT_EQ("struct foo {\n"
+"#ifdef FOO\n"
+"private:\n"
+"#endif\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"
+   "#ifdef FOO\n"
+   "private:\n"
+   "#endif\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   getLLVMStyle()));
+  FormatStyle Style = getLLVMStyle();
+  Style.EmptyLineBeforeAccessModifier = false;
+  EXPECT_EQ("struct foo {\n"
+"  int i;\n"
+"private:\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"
+   "  int i;\n"
+   "\n"
+   "private:\n"
+   "  int j;\n"
+   "}\n",
+   Style));
+  EXPECT_EQ("struct foo {\n"
+"private:\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   Style));
+  EXPECT_EQ("struct foo {\n"
+"private:\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "  int j;\n"
+   "}\n",
+   Style));
+  EXPECT_EQ("struct foo {\n"
+"  // comment\n"
+"private:\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+format("struct foo {\n"
+   "  // comment\n"
+   "\n"
+   

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/include/clang/Format/Format.h:68
+  /// \endcode
+  bool InsertEmptyLineBeforeAccessModifier;
+

HazardyKnusperkeks wrote:
> MyDeveloperDay wrote:
> > quite a mouthful... maybe just `NewLineBeforeAccessModifier` ?
> A new line is always there, how about `EmptyLineBeforeAccessModifier`?
> 
> Apart from that, I would prefer an alphabetical sorting of the member, most 
> of them are (not all I know...).

Yes `EmptyLine` seems the better termininology


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/include/clang/Format/Format.h:68
+  /// \endcode
+  bool InsertEmptyLineBeforeAccessModifier;
+

MyDeveloperDay wrote:
> quite a mouthful... maybe just `NewLineBeforeAccessModifier` ?
A new line is always there, how about `EmptyLineBeforeAccessModifier`?

Apart from that, I would prefer an alphabetical sorting of the member, most of 
them are (not all I know...).



Comment at: clang/include/clang/Format/Format.h:2370
IndentWrappedFunctionNames == R.IndentWrappedFunctionNames &&
+   InsertEmptyLineBeforeAccessModifier ==
+   R.InsertEmptyLineBeforeAccessModifier &&

Here it is sorted.



Comment at: clang/lib/Format/Format.cpp:414
 IO.mapOptional("AccessModifierOffset", Style.AccessModifierOffset);
+IO.mapOptional("InsertEmptyLineBeforeAccessModifier",
+   Style.InsertEmptyLineBeforeAccessModifier);

Here again not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93846

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

[PATCH] D93846: [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

My 2 cents. Otherwise I agree with @MyDeveloperDay's comments.




Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1219-1228
+  if (Style.InsertEmptyLineBeforeAccessModifier && PreviousLine &&
+  PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
   RootToken.isAccessSpecifier() && RootToken.NewlinesBefore == 1)
 ++Newlines;
 
+  // Remove empty lines before access specifiers.
+  if (!Style.InsertEmptyLineBeforeAccessModifier && PreviousLine &&

Given that the two `if`s handle the same construct, I think we should try to 
merge them to avoid duplication in conditions. Sth like:
```
if (PreviousLine && RootToken.isAccessSpecifier()) {
if (... Style.Insert ... && ... ) {
...
}
else if ( (... !Style.Insert ... && ... ) {
...
}
}
```



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1220
+  if (Style.InsertEmptyLineBeforeAccessModifier && PreviousLine &&
+  PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
   RootToken.isAccessSpecifier() && RootToken.NewlinesBefore == 1)

Just thinking out loud, but shouldn't we just check that 
`PreviousLine->Last->isNot(tok::l_brace)`?
Could you please add a test with comments before access modifiers?



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1226
+  if (!Style.InsertEmptyLineBeforeAccessModifier && PreviousLine &&
+  PreviousLine->Last->isOneOf(tok::semi, tok::r_brace, tok::l_brace) &&
+  RootToken.isAccessSpecifier() && RootToken.NewlinesBefore > 1)

Is it really needed to check the type of `PreviousLine->Last`?



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1231-1232
   // Remove empty lines after access specifiers.
   if (PreviousLine && PreviousLine->First->isAccessSpecifier() &&
   (!PreviousLine->InPPDirective || !RootToken.HasUnescapedNewline))
 Newlines = std::min(1u, Newlines);

Please add tests with preprocessor directives *before* access modifiers too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93846

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