[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Max Sagebaum via Phabricator via cfe-commits]

Max_S added a comment.

In D98237#2775514 , @MyDeveloperDay 
wrote:

> When you made this commit you didn't regenerate the 
> ClangFormatStyleOptions.rst from the Format.h using 
> clang/docs/tool/dump_format_style.py now anyone else using the tool hit 
> issues with code which is incorrect, we need to update Format.h to match what 
> you expect in ClangFormatStyleOptions.rst

I am sorry about this. I provided a bugfix in D102989 
, can you please have a look if this is 
enough?

Is there a documentation I forget to read?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

When you made this commit you didn't regenerate the ClangFormatStyleOptions.rst 
from the Format.h using clang/docs/tool/dump_format_style.py now anyone else 
using the tool hit issues with code which is incorrect, we need to update 
Format.h to match what you expect in ClangFormatStyleOptions.rst


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Marek Kurdej via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdda978eef87c: [clang-format] Option for empty lines after an 
access modifier. (authored by Max_S, committed by curdeius).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -9179,6 +9179,554 @@
Style);
 }
 
+TEST_F(FormatTest, FormatsAfterAccessModifiers) {
+
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.EmptyLineAfterAccessModifier, FormatStyle::ELAAMS_Never);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+
+  // Check if lines are removed.
+  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"
+   "\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+
+  Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Always;
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+
+  // Check if lines are added.
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+
+  // Leave tests rely on the code layout, test::messUp can not be used.
+  Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Leave;
+  Style.MaxEmptyLinesToKeep = 0u;
+  EXPECT_EQ("struct foo {\n"
+"private:\n"
+"  void f() {}\n"
+"private:\n"
+"  int i;\n"
+"protected:\n"
+"  int j;\n"
+"};\n",
+format("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style));
+
+  // Check if MaxEmptyLinesToKeep is respected.
+  EXPECT_EQ("struct foo {\n"
+"private:\n"
+"  void f() {}\n"
+"private:\n"
+"  int i;\n"
+"protected:\n"
+"  int j;\n"
+"};\n",
+format("struct foo {\n"
+   "private:\n"
+   "\n\n\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "\n\n\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "\n\n\n"
+   "  int j;\n"
+   "};\n",
+   Style));
+
+  Style.MaxEmptyLinesToKeep = 1u;
+  EXPECT_EQ("struct foo {\n"
+"private:\n"
+"\n"
+"  void f() {}\n"
+"\n"
+"private:\n"
+"\n"
+"  int i;\n"
+"\n"
+"protected:\n"
+"\n"
+"  int j;\n"
+"};\n",
+format("struct foo {\n"
+   

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Max Sagebaum via Phabricator via cfe-commits]

Max_S added a comment.

Thank you for landing it. The information should be ok. But anyway: Max 
Sagebaum 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

I'll land it for you. Could you please provide "Name Surname " for 
commit attribution unless the info associated with your phabricator user is 
okay?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Max Sagebaum via Phabricator via cfe-commits]

Max_S added a comment.

Thank you for the answer. It did not know that I have to land it myself.

Now I read up on https://llvm.org/docs/Phabricator.html

Tried to land via 'arc land' but I do not have the access rights.

So: Can someone please land the change for me. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

I think you can land it. The issues seem indeed unrelated.
You might want to rebase and retest before landing to be sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Max Sagebaum via Phabricator via cfe-commits]

Max_S added a comment.

Just wanted to ask if there is something missing for the merge?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Max Sagebaum via Phabricator via cfe-commits]

Max_S updated this revision to Diff 334523.
Max_S marked 7 inline comments as done.
Max_S added a comment.

Addressed last minor remarks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -9179,6 +9179,554 @@
Style);
 }
 
+TEST_F(FormatTest, FormatsAfterAccessModifiers) {
+
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.EmptyLineAfterAccessModifier, FormatStyle::ELAAMS_Never);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+
+  // Check if lines are removed.
+  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"
+   "\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+
+  Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Always;
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+
+  // Check if lines are added.
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+
+  // Leave tests rely on the code layout, test::messUp can not be used.
+  Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Leave;
+  Style.MaxEmptyLinesToKeep = 0u;
+  EXPECT_EQ("struct foo {\n"
+"private:\n"
+"  void f() {}\n"
+"private:\n"
+"  int i;\n"
+"protected:\n"
+"  int j;\n"
+"};\n",
+format("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style));
+
+  // Check if MaxEmptyLinesToKeep is respected.
+  EXPECT_EQ("struct foo {\n"
+"private:\n"
+"  void f() {}\n"
+"private:\n"
+"  int i;\n"
+"protected:\n"
+"  int j;\n"
+"};\n",
+format("struct foo {\n"
+   "private:\n"
+   "\n\n\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "\n\n\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "\n\n\n"
+   "  int j;\n"
+   "};\n",
+   Style));
+
+  Style.MaxEmptyLinesToKeep = 1u;
+  EXPECT_EQ("struct foo {\n"
+"private:\n"
+"\n"
+"  void f() {}\n"
+"\n"
+"private:\n"
+"\n"
+"  int i;\n"
+"\n"
+"protected:\n"
+"\n"
+"  int j;\n"
+"};\n",
+format("struct foo {\n"
+   "private:\n"
+   "\n"
+   "  void f() {}\n"
+   "\n"
+   "pri

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.
This revision is now accepted and ready to land.

LGTM, but please clean up the comments before landing.




Comment at: clang/docs/ClangFormatStyleOptions.rst:2132
+**EmptyLineAfterAccessModifier** (``EmptyLineAfterAccessModifierStyle``)
+  Defines in which cases to put empty line after access modifiers.
+  ``EmptyLineBeforeAccessModifier`` configuration handles the number of

Please apply this when landing.



Comment at: clang/unittests/Format/FormatTest.cpp:9289
+
+  // Check if MaxEmptyLinesToKeep is adhered
+  EXPECT_EQ("struct foo {\n"

Nit. Here and elsewhere.



Comment at: clang/unittests/Format/FormatTest.cpp:9546
+
+  Style.MaxEmptyLinesToKeep = 10u; // Both remove all new lines
+  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Never;

Please rephrase/move the comment to be clear that it concerns ELBAMS/ELAAMS.



Comment at: clang/unittests/Format/FormatTest.cpp:9598
+   "};\n",
+   Style)); // Based on new lines in orignal document and not 
on
+// the setting

Nit.



Comment at: clang/unittests/Format/FormatTest.cpp:9605
+  // Newlines are keept if they are greater than zero,
+  // test::messUp removes all new lines which changes the logic
+  EXPECT_EQ("struct foo {\n"

Please end comments with full stops. Here and elsewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.

LGTM thank you for trying where you could to use verifyFormat


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Max Sagebaum via Phabricator via cfe-commits]

Max_S marked 9 inline comments as done.
Max_S added a comment.

In the last update I fixed now everything and changed the tests.

The tests use now verify format when possible. A bug here, kept me from doing 
it in a few places. I submitted a bug report for this: 
https://bugs.llvm.org/show_bug.cgi?id=49772

I hope this is now fine and can be merged into the master.




Comment at: clang/include/clang/Format/Format.h:1912
+/// Keep existing empty lines after access modifiers.
+/// MaxEmptyLinesToKeep is applied instead.
+ELAAMS_Leave,

curdeius wrote:
> Shouldn't we put the same comment in `ELBAMS_Leave`?
> That goes outside the scope of this patch if it ELBAMS doesn't behave this 
> way.
I submitted a bug report about this, since the behavior is different.

https://bugs.llvm.org/show_bug.cgi?id=49757


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Max Sagebaum via Phabricator via cfe-commits]

Max_S updated this revision to Diff 334186.
Max_S added a comment.

Cleanup of tests and additonal logic for modifiers at the end of a class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -9179,6 +9179,555 @@
Style);
 }
 
+TEST_F(FormatTest, FormatsAfterAccessModifiers) {
+
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.EmptyLineAfterAccessModifier, FormatStyle::ELAAMS_Never);
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+
+  // Check if lines are removed
+  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"
+   "\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+
+  Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Always;
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+
+  // Check if lines are added
+  verifyFormat("struct foo {\n"
+   "private:\n"
+   "\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "\n"
+   "  int j;\n"
+   "};\n",
+   "struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style);
+
+  // Leave tests rely on the code layout, test::messUp can not be used.
+  Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Leave;
+  Style.MaxEmptyLinesToKeep = 0u;
+  EXPECT_EQ("struct foo {\n"
+"private:\n"
+"  void f() {}\n"
+"private:\n"
+"  int i;\n"
+"protected:\n"
+"  int j;\n"
+"};\n",
+format("struct foo {\n"
+   "private:\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "  int j;\n"
+   "};\n",
+   Style));
+
+  // Check if MaxEmptyLinesToKeep is adhered
+  EXPECT_EQ("struct foo {\n"
+"private:\n"
+"  void f() {}\n"
+"private:\n"
+"  int i;\n"
+"protected:\n"
+"  int j;\n"
+"};\n",
+format("struct foo {\n"
+   "private:\n"
+   "\n\n\n"
+   "  void f() {}\n"
+   "\n"
+   "private:\n"
+   "\n\n\n"
+   "  int i;\n"
+   "\n"
+   "protected:\n"
+   "\n\n\n"
+   "  int j;\n"
+   "};\n",
+   Style));
+
+  Style.MaxEmptyLinesToKeep = 1u;
+  EXPECT_EQ("struct foo {\n"
+"private:\n"
+"\n"
+"  void f() {}\n"
+"\n"
+"private:\n"
+"\n"
+"  int i;\n"
+"\n"
+"protected:\n"
+"\n"
+"  int j;\n"
+"};\n",
+format("struct foo {\n"
+   "private:\n"
+   "\n"
+   "  void f() {}\n"
+   "\n"
+   "priv

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Max Sagebaum via Phabricator via cfe-commits]

Max_S added a comment.

In D98237#2643815 , @MyDeveloperDay 
wrote:

> If you follow people tweeting about clang-format (as I do) and you look 
> through the bug tracking system, one major criticism of clang-format is that 
> the second clang-format can be different from the first, sometimes an 
> equilibrium can be found sometimes not.
>
> When I started working on clang-format I was encouraged to use verifyFormat() 
> as it tests that scenario and also tries to mess up the format and ensure it 
> returns to the desired state. It found bugs in my code which would have made 
> clang-format worse.
>
> Apart from it being the convention I believe it makes for much more readable 
> code, even if there is repetition as I don't need to keep cross referencing 
> variables with rather obscure names `NL_B_3_A_0_I_0` this is unnecessary 
> noise and makes the code overly verbose.
>
> No you'll need to check out what the messUp() function is actually doing but 
> I think by and large IMHO we should stick with verifyFormat.

Ok then I will change the tests accordingly. This reasoning should be written 
down somewhere.

In D98237#2643880 , @MyDeveloperDay 
wrote:

> I'd be quite interested to understand what the impact (if any) would be on 
> javascript and C# formatting

I have not done anything in javascript a quick google search showed no such 
modifiers in classes. Do you have an example? C# seems to handle it like java 
and then the modifiers become properties of the functions member, there should 
be no influence.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I'd be quite interested to understand what the impact (if any) would be on 
javascript and C# formatting


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

If you follow people tweeting about clang-format (as I do) and you look through 
the bug tracking system, one major criticism of clang-format is that the second 
clang-format can be different from the first, sometimes an equilibrium can be 
found sometimes not.

When I started working on clang-format I was encouraged to use verifyFormat() 
as it tests that scenario and also tries to mess up the format and ensure it 
returns to the desired state. It found bugs in my code which would have made 
clang-format worse.

Apart from it being the convention I believe it makes for much more readable 
code, even if there is repetition as I don't need to keep cross referencing 
variables with rather obscure names `NL_B_3_A_0_I_0` this is unnecessary noise 
and makes the code overly verbose.

No you'll need to check out what the messUp() function is actually doing but I 
think by and large IMHO we should stick with verifyFormat.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Max Sagebaum via Phabricator via cfe-commits]

Max_S marked 4 inline comments as done.
Max_S added a comment.

> The tests in line 9462 and 9466 require some additional implementation. 
> EmptyLineAfterAccessModifier is adding the three lines since it is configured 
> to do so, but EmptyLineBeforeAccessModifier would remove these lines. Since 
> EmptyLineBeforeAccessModifier can only access the old file and not the new 
> one. It does not know that three lines have been written and probably could 
> not remove them.
>
> I would like to implement it in such a way, that EmptyLineAfterAccessModifier 
> looks ahead and skips its logic if the next token is a also an access 
> modifier such that the logic of EmptyLineBeforeAccessModifier takes 
> precedence. I could not find a way to get the next line. Is this possible 
> somehow? Does there exist some helper functionality?

I fixed this in the  newest change set. EmptyLineBeforeAccessModifier handles 
the case when two access modifiers follow each other. I updated the 
documentation accordingly.

> Four of the new tests fail, two of them are probably bugs in the 
> implementation of EmptyLineBeforeAccessModifier.
>
> The test in line 9354 is set to:
>
>   Style.MaxEmptyLinesToKeep = 0u; // Both force one new line
>   Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Always;
>   Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Always;
>   EXPECT_EQ(NL_B_1_A_1_I_1, format(NL_B_3_A_3_I_3, Style));
>
> So both options should force here the newline, that are removed by 
> MaxEmptyLinesToKeep. EmptyLineBeforeAccessModifier removes all lines, so it 
> seems that MaxEmptyLinesToKeep has the higher rank here. Is that correct 
> behavior?
>
> The test in line 9368 is set to:
>
>   Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Leave;
>   Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Leave;
>   Style.MaxEmptyLinesToKeep = 1u; 
>   EXPECT_EQ(NL_B_1_A_1_I_1, format(NL_B_3_A_3_I_3, Style));
>
> So both options should leave the new lines in place but should adhere to 
> MaxEmptyLinesToKeep. EmptyLineBeforeAccessModifier keeps all three lines, so 
> it seems that MaxEmptyLinesToKeep has the lower rank here. Is that correct 
> behavior?

I still need some advice on how to handle this. Should I fix the test so that 
it captures the current behavior. Should I fix the logic or should I open a bug 
report?

In D98237#2637832 , @MyDeveloperDay 
wrote:

> could you please mark your comments done when they are done.

I usually try to do this. How can I see which ones are still open?




Comment at: clang/docs/ClangFormatStyleOptions.rst:2159
+Always add empty line after access modifiers if there are none.
+MaxEmptyLinesToKeep is applied also.
+

MyDeveloperDay wrote:
> if MaxEmptyLineToKeep is 0 then surely ELAAMS_Always should win, as this is 
> the lesser i.e. if people never uses extra lines except after access 
> modifiers this is what they would set? did I missunderstand?
> 
> otherwise doesn't MaxEmptyLinesToKeep just nullify the whole option? meaning 
> its useless for anyone using it?
> 
> I agree in the ELAAMS_Leave  case that MaxEmptyLinesToKeep  should win.
> if MaxEmptyLineToKeep is 0 then surely ELAAMS_Always should win, as this is 
> the lesser i.e. if people never uses extra lines except after access 
> modifiers this is what they would set? did I missunderstand?

You understood correctly and that is the way it is implemented.

> I agree in the ELAAMS_Leave case that MaxEmptyLinesToKeep should win.

That is also the way it is implemented.

I fixed the problem with two access modifiers in a row. 



Comment at: clang/unittests/Format/FormatTest.cpp:9392
+ "};\n";
+  EXPECT_EQ(NL_B_3_A_1_I_3, format(NL_B_3_A_3_I_3, Style));
+

MyDeveloperDay wrote:
> I can't read this, I can't tell if you've fat fingered a test, I feel these 
> are better done systematically one by one with the text in the 
> verifyFormat("...") it makes it easier when they fail to understand what is 
> going on.
> 
> Also its ok to have more than one TEST_F if you want to handle the different 
> states in small batches
> 
> I think you undetersand what NL_B_3_A_0_I_0 means and how it differs form 
> NL_B_0_A_3_I_0  but others won't
> I can't read this, 
I may have missed here a comment. The convention is:
```
NewLines_Before_Access_Modifier_3_After_Access_Modifier_3_Inbetween_Access_Modifier_3
N L_   B_   3_A_
3_I_   3
NL_B_3_A_3_I_3
```

> I can't tell if you've fat fingered a test
What do you mean by that?

> I feel these are better done systematically
Thats what I have actually done. First tests only concerning 
EmptyLineAfterAccessModifier afterwards the combinations with 
EmptyLineAfterAccessModifier and EmptyLineBeforeAccessModifier.
You h

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Max Sagebaum via Phabricator via cfe-commits]

Max_S updated this revision to Diff 332236.
Max_S added a comment.

Fixed interaction between Before and After configurations options. Before 
handles the case
of two access modifiers specification following each other.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -9179,6 +9179,296 @@
Style);
 }
 
+TEST_F(FormatTest, FormatsAfterAccessModifiers) {
+  char const *const NL_After_Never = "struct foo {\n"
+ "private:\n"
+ "  void f() {}\n"
+ "\n"
+ "private:\n"
+ "  int i;\n"
+ "\n"
+ "protected:\n"
+ "  int j;\n"
+ "};\n";
+  char const *const NL_After_Always = "struct foo {\n"
+  "private:\n"
+  "\n"
+  "  void f() {}\n"
+  "\n"
+  "private:\n"
+  "\n"
+  "  int i;\n"
+  "\n"
+  "protected:\n"
+  "\n"
+  "  int j;\n"
+  "};\n";
+
+  char const *const NL_After_Always3Times = "struct foo {\n"
+"private:\n"
+"\n\n\n"
+"  void f() {}\n"
+"\n"
+"private:\n"
+"\n\n\n"
+"  int i;\n"
+"\n"
+"protected:\n"
+"\n\n\n"
+"  int j;\n"
+"};\n";
+
+  char const *const NL_After_Never_Before_Never = "struct foo {\n"
+  "private:\n"
+  "  void f() {}\n"
+  "private:\n"
+  "  int i;\n"
+  "protected:\n"
+  "  int j;\n"
+  "};\n";
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.EmptyLineAfterAccessModifier, FormatStyle::ELAAMS_Never);
+  EXPECT_EQ(NL_After_Never, format(NL_After_Never, Style));
+  EXPECT_EQ(NL_After_Never, format(NL_After_Always, Style));
+
+  Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Always;
+  EXPECT_EQ(NL_After_Always, format(NL_After_Never, Style));
+  EXPECT_EQ(NL_After_Always, format(NL_After_Always, Style));
+
+  Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Leave;
+  Style.MaxEmptyLinesToKeep = 0u;
+  EXPECT_EQ(NL_After_Never_Before_Never, format(NL_After_Never, Style));
+  EXPECT_EQ(NL_After_Never_Before_Never, format(NL_After_Always, Style));
+  EXPECT_EQ(NL_After_Never_Before_Never, format(NL_After_Always3Times, Style));
+
+  Style.MaxEmptyLinesToKeep = 1u;
+  EXPECT_EQ(NL_After_Never, format(NL_After_Never, Style));
+  EXPECT_EQ(NL_After_Always, format(NL_After_Always, Style));
+  EXPECT_EQ(NL_After_Always, format(NL_After_Always3Times, Style));
+
+  Style.MaxEmptyLinesToKeep = 10u;
+  EXPECT_EQ(NL_After_Never, format(NL_After_Never, Style));
+  EXPECT_EQ(NL_After_Always, format(NL_After_Always, Style));
+  EXPECT_EQ(NL_After_Always3Times, format(NL_After_Always3Times, Style));
+
+  // Test with comments
+  char const *const NL_Com_After_Never = "struct foo {\n"
+ "private:\n"
+ "  // comment\n"
+ "  void f() {}\n"
+ "\n"
+ "private: /* comment */\n"
+ "  int i;\n"
+ "

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:2132
+**EmptyLineAfterAccessModifier** (``EmptyLineAfterAccessModifierStyle``)
+  Defines in which cases to put empty line after access modifiers.
+





Comment at: clang/docs/ClangFormatStyleOptions.rst:2159
+Always add empty line after access modifiers if there are none.
+MaxEmptyLinesToKeep is applied also.
+

if MaxEmptyLineToKeep is 0 then surely ELAAMS_Always should win, as this is the 
lesser i.e. if people never uses extra lines except after access modifiers this 
is what they would set? did I missunderstand?

otherwise doesn't MaxEmptyLinesToKeep just nullify the whole option? meaning 
its useless for anyone using it?

I agree in the ELAAMS_Leave  case that MaxEmptyLinesToKeep  should win.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

could you please mark your comments done when they are done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay requested changes to this revision.
MyDeveloperDay added inline comments.
This revision now requires changes to proceed.



Comment at: clang/unittests/Format/FormatTest.cpp:9392
+ "};\n";
+  EXPECT_EQ(NL_B_3_A_1_I_3, format(NL_B_3_A_3_I_3, Style));
+

I can't read this, I can't tell if you've fat fingered a test, I feel these are 
better done systematically one by one with the text in the verifyFormat("...") 
it makes it easier when they fail to understand what is going on.

Also its ok to have more than one TEST_F if you want to handle the different 
states in small batches

I think you undetersand what NL_B_3_A_0_I_0 means and how it differs form 
NL_B_0_A_3_I_0  but others won't


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:9212
+  StyleWithLine.EmptyLinesAfterAccessModifier = 1u;
+  EXPECT_EQ(test2NL, format(test0NL, StyleWithLine));
+  EXPECT_EQ(test2NL, format(test1NL, StyleWithLine));

Max_S wrote:
> HazardyKnusperkeks wrote:
> > Max_S wrote:
> > > MyDeveloperDay wrote:
> > > > yeah I'm not a fan of this like this... sorry... just write the test 
> > > > out in long form, when it goes wrong I don't have to be a compiler to 
> > > > understand what is going wrong I can just see it.
> > > I can change this, but the current output of the tests is (I forced the 
> > > error):
> > > ```
> > > //llvm-project/clang/unittests/Format/FormatTest.cpp:72: Failure
> > >   Expected: Expected.str()
> > >   Which is: "class Foo {\nprivate:\n\n  int i;\n};"
> > > To be equal to: format(Expected, Style)
> > >   Which is: "class Foo {\nprivate:\n  int i;\n};"
> > > With diff:
> > > @@ -1,5 @@
> > >  class Foo {
> > >  private:
> > > -
> > >int i;
> > >  };
> > > ```
> > > 
> > > Which is actually human readable in this case. Shall I still change it?
> > I'm a fan of it. :)
> > Especially with the demonstration that the string is still expanded.
> I changed all to EXPECT_EQ since the failer output there is better. It shows 
> the variables names and not the arbitrary code in verfyFormat. Hope this is 
> ok.
> ```
> /home/msagebaum/Kaiserslautern/Programms/temp/llvm-project/clang/unittests/Format/FormatTest.cpp:9202:
>  Failure
>   Expected: test2NL
>   Which is: "class Foo {\nprivate:\n\n  int i;\n};"
> To be equal to: format(test1NL)
>   Which is: "class Foo {\nprivate:\n  int i;\n};"
> With diff:
> @@ -1,5 @@
>  class Foo {
>  private:
> -
>int i;
>  };
> 
> 
why would we break with convention?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Max Sagebaum via Phabricator via cfe-commits]

Max_S added inline comments.



Comment at: clang/include/clang/Format/Format.h:1914
+ELAAMS_Leave,
+/// Always add empty line after access modifiers.
+/// \code

HazardyKnusperkeks wrote:
> It does not always add, it does enforces one empty line, or am I mistaken?
It adds it if it is missing, but leaves additional lines. All in all 
MaxEmptyLinesToKeep has precedence here. If MaxEmptyLinesToKeep is zero, then 
no line is added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Max Sagebaum via Phabricator via cfe-commits]

Max_S updated this revision to Diff 331818.
Max_S added a comment.

I added more tests for the interaction between MaxEmptyLinesToKeep, 
EmptyLineBeforeAccessModifier and EmptyLineAfterAccessModifier. During these 
tests I recognized, that EmptyLineBeforeAccessModifier = Always,Leave will 
(should adhere) to MaxEmptyLinesToKeep. So the option does not force the new 
line it only applies it if it is missing. I changed the logic for 
EmptyLineAfterAccessModifier accordingly.

Four of the new tests fail, two of them are probably bugs in the implementation 
of EmptyLineBeforeAccessModifier.

The test in line 9354 is set to:

  Style.MaxEmptyLinesToKeep = 0u; // Both force one new line
  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Always;
  Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Always;
  EXPECT_EQ(NL_B_1_A_1_I_1, format(NL_B_3_A_3_I_3, Style));

So both options should force here the newline, that are removed by 
MaxEmptyLinesToKeep. EmptyLineBeforeAccessModifier removes all lines, so it 
seems that MaxEmptyLinesToKeep has the higher rank here. Is that correct 
behavior?

The test in line 9368 is set to:

  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Leave;
  Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Leave;
  Style.MaxEmptyLinesToKeep = 1u; 
  EXPECT_EQ(NL_B_1_A_1_I_1, format(NL_B_3_A_3_I_3, Style));

So both options should leave the new lines in place but should adhere to 
MaxEmptyLinesToKeep. EmptyLineBeforeAccessModifier keeps all three lines, so it 
seems that MaxEmptyLinesToKeep has the lower rank here. Is that correct 
behavior?

The tests in line 9462 and 9466 require some additional implementation. 
EmptyLineAfterAccessModifier is adding the three lines since it is configured 
to do so, but EmptyLineBeforeAccessModifier would remove these lines. Since 
EmptyLineBeforeAccessModifier can only access the old file and not the new one. 
It does not know that three lines have been written and probably could not 
remove them.

I would like to implement it in such a way, that EmptyLineAfterAccessModifier 
looks ahead and skips its logic if the next token is a also an access modifier 
such that the logic of EmptyLineBeforeAccessModifier takes precedence. I could 
not find a way to get the next line. Is this possible somehow? Does there exist 
some helper functionality?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -9179,6 +9179,297 @@
Style);
 }
 
+TEST_F(FormatTest, FormatsAfterAccessModifiers) {
+  char const *const NL_After_Never = "struct foo {\n"
+ "private:\n"
+ "  void f() {}\n"
+ "\n"
+ "private:\n"
+ "  int i;\n"
+ "\n"
+ "protected:\n"
+ "  int j;\n"
+ "};\n";
+  char const *const NL_After_Always = "struct foo {\n"
+  "private:\n"
+  "\n"
+  "  void f() {}\n"
+  "\n"
+  "private:\n"
+  "\n"
+  "  int i;\n"
+  "\n"
+  "protected:\n"
+  "\n"
+  "  int j;\n"
+  "};\n";
+
+  char const *const NL_After_Always3Times = "struct foo {\n"
+"private:\n"
+"\n\n\n"
+"  void f() {}\n"
+"\n"
+"private:\n"
+"\n\n\n"
+"  int i;\n"
+"\n"
+"protected:\n"
+"\n\n\n"
+"  int j;\n"
+"};\n";
+
+  char const *const NL_After_Never_Before_Never = "struct foo {\n"
+ 

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

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

HazardyKnusperkeks added inline comments.



Comment at: clang/include/clang/Format/Format.h:1914
+ELAAMS_Leave,
+/// Always add empty line after access modifiers.
+/// \code

It does not always add, it does enforces one empty line, or am I mistaken?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

That looks really good. Please add tests as indicated in the inline comments 
and fix the formatting (see clang-format warnings).




Comment at: clang/include/clang/Format/Format.h:1912
+/// Keep existing empty lines after access modifiers.
+/// MaxEmptyLinesToKeep is applied instead.
+ELAAMS_Leave,

Shouldn't we put the same comment in `ELBAMS_Leave`?
That goes outside the scope of this patch if it ELBAMS doesn't behave this way.



Comment at: clang/include/clang/Format/Format.h:1957
+  /// Defines how many lines are put after access modifiers.
+  unsigned EmptyLinesAfterAccessModifier;
+

Max_S wrote:
> curdeius wrote:
> > This option seems to be very different from 
> > `EmptyLineBeforeAccessModifier`. I don't mean in what it does, because this 
> > is analogical, but in the possible options.
> > Wouldn't it be less surprising to have (at least some) similar options here 
> > and there?
> > Is there any value in having more than one line after access modifiers? 
> > Couldn't that be achieved with Leave option?
> > How do the two options work together?
> > 
> > Also, the difference in singular vs. plural form of "Line(s)" in these two 
> > options is disconcerting.
> > From the user perspective, it's error-prone to have two options that are at 
> > the same time so similar and so different.
> I agree with you and changed it accordingly. I left out the option 
> `LogicalBlock`.
> 
> The interaction between the two options is quite minimal. I can add extra 
> tests, that would demonstrate this but I do not think that this is necessary.
> 
> The leave option would now applies MaxEmptyLinesToKeep in a correct way. See 
> also my remarks in >>! In D98237#2616621.
I like the new version. Thank you for the update.
I'd still love to see the tests with both Before/After options (and we probably 
want `MaxEmptyLinesToKeep > 1` for these tests in order to check whether e.g. 
Always/Always keeps precisely one new line).

I mean something along these lines (amongst other tests):
```
  Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Always;
  Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Always;
  Style.MaxEmptyLinesToKeep = 3;
  EXPECT_EQ(R"(
struct S {
private:

public:
};
)",
format(R"(
struct S {
private:



public:
};
)", Style));
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Max Sagebaum via Phabricator via cfe-commits]

Max_S added a comment.

As already described in the update I changed the option name and how it 
behaves. It is now more in line with EmptyLineBeforeAccessModifier.

I hope this was the correct way to push the rewrite, since all line remarks are 
now off.




Comment at: clang/include/clang/Format/Format.h:1957
+  /// Defines how many lines are put after access modifiers.
+  unsigned EmptyLinesAfterAccessModifier;
+

curdeius wrote:
> This option seems to be very different from `EmptyLineBeforeAccessModifier`. 
> I don't mean in what it does, because this is analogical, but in the possible 
> options.
> Wouldn't it be less surprising to have (at least some) similar options here 
> and there?
> Is there any value in having more than one line after access modifiers? 
> Couldn't that be achieved with Leave option?
> How do the two options work together?
> 
> Also, the difference in singular vs. plural form of "Line(s)" in these two 
> options is disconcerting.
> From the user perspective, it's error-prone to have two options that are at 
> the same time so similar and so different.
I agree with you and changed it accordingly. I left out the option 
`LogicalBlock`.

The interaction between the two options is quite minimal. I can add extra 
tests, that would demonstrate this but I do not think that this is necessary.

The leave option would now applies MaxEmptyLinesToKeep in a correct way. See 
also my remarks in >>! In D98237#2616621.



Comment at: clang/unittests/Format/FormatTest.cpp:9212
+  StyleWithLine.EmptyLinesAfterAccessModifier = 1u;
+  EXPECT_EQ(test2NL, format(test0NL, StyleWithLine));
+  EXPECT_EQ(test2NL, format(test1NL, StyleWithLine));

HazardyKnusperkeks wrote:
> Max_S wrote:
> > MyDeveloperDay wrote:
> > > yeah I'm not a fan of this like this... sorry... just write the test out 
> > > in long form, when it goes wrong I don't have to be a compiler to 
> > > understand what is going wrong I can just see it.
> > I can change this, but the current output of the tests is (I forced the 
> > error):
> > ```
> > //llvm-project/clang/unittests/Format/FormatTest.cpp:72: Failure
> >   Expected: Expected.str()
> >   Which is: "class Foo {\nprivate:\n\n  int i;\n};"
> > To be equal to: format(Expected, Style)
> >   Which is: "class Foo {\nprivate:\n  int i;\n};"
> > With diff:
> > @@ -1,5 @@
> >  class Foo {
> >  private:
> > -
> >int i;
> >  };
> > ```
> > 
> > Which is actually human readable in this case. Shall I still change it?
> I'm a fan of it. :)
> Especially with the demonstration that the string is still expanded.
I changed all to EXPECT_EQ since the failer output there is better. It shows 
the variables names and not the arbitrary code in verfyFormat. Hope this is ok.
```
/home/msagebaum/Kaiserslautern/Programms/temp/llvm-project/clang/unittests/Format/FormatTest.cpp:9202:
 Failure
  Expected: test2NL
  Which is: "class Foo {\nprivate:\n\n  int i;\n};"
To be equal to: format(test1NL)
  Which is: "class Foo {\nprivate:\n  int i;\n};"
With diff:
@@ -1,5 @@
 class Foo {
 private:
-
   int i;
 };




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Max Sagebaum via Phabricator via cfe-commits]

Max_S updated this revision to Diff 330588.
Max_S added a comment.

Changed the option to EmptyLineAfterAccessModifier and allowed the options 
Never, Leave and Always. Updated the tests accordingly and also added an entry 
in the changelog.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -9179,6 +9179,128 @@
Style);
 }
 
+TEST_F(FormatTest, FormatsAfterAccessModifiers) {
+  char const *const NL_After_Never = "struct foo {\n"
+ "private:\n"
+ "  void f() {}\n"
+ "\n"
+ "private:\n"
+ "  int i;\n"
+ "\n"
+ "protected:\n"
+ "  int j;\n"
+ "};\n";
+  char const *const NL_After_Always = "struct foo {\n"
+  "private:\n"
+  "\n"
+  "  void f() {}\n"
+  "\n"
+  "private:\n"
+  "\n"
+  "  int i;\n"
+  "\n"
+  "protected:\n"
+  "\n"
+  "  int j;\n"
+  "};\n";
+
+  char const *const NL_After_Always3Times = "struct foo {\n"
+"private:\n"
+"\n\n\n"
+"  void f() {}\n"
+"\n"
+"private:\n"
+"\n\n\n"
+"  int i;\n"
+"\n"
+"protected:\n"
+"\n\n\n"
+"  int j;\n"
+"};\n";
+
+  char const *const NL_After_Never_Before_Never = "struct foo {\n"
+  "private:\n"
+  "  void f() {}\n"
+  "private:\n"
+  "  int i;\n"
+  "protected:\n"
+  "  int j;\n"
+  "};\n";
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.EmptyLineAfterAccessModifier, FormatStyle::ELAAMS_Never);
+  EXPECT_EQ(NL_After_Never, format(NL_After_Never, Style));
+  EXPECT_EQ(NL_After_Never, format(NL_After_Always, Style));
+
+  Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Always;
+  EXPECT_EQ(NL_After_Always, format(NL_After_Never, Style));
+  EXPECT_EQ(NL_After_Always, format(NL_After_Always, Style));
+
+  Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Leave;
+  Style.MaxEmptyLinesToKeep = 0u;
+  EXPECT_EQ(NL_After_Never_Before_Never, format(NL_After_Never, Style));
+  EXPECT_EQ(NL_After_Never_Before_Never, format(NL_After_Always, Style));
+  EXPECT_EQ(NL_After_Never_Before_Never, format(NL_After_Always3Times, Style));
+
+  Style.MaxEmptyLinesToKeep = 1u;
+  EXPECT_EQ(NL_After_Never, format(NL_After_Never, Style));
+  EXPECT_EQ(NL_After_Always, format(NL_After_Always, Style));
+  EXPECT_EQ(NL_After_Always, format(NL_After_Always3Times, Style));
+
+  Style.MaxEmptyLinesToKeep = 10u;
+  EXPECT_EQ(NL_After_Never, format(NL_After_Never, Style));
+  EXPECT_EQ(NL_After_Always, format(NL_After_Always, Style));
+  EXPECT_EQ(NL_After_Always3Times, format(NL_After_Always3Times, Style));
+
+  // Test with comments
+  char const *const NL_Com_After_Never = "struct foo {\n"
+ "private:\n"
+ "  // comment\n"
+ "  void f() {}\n"
+ "\n"
+ "private: /* comment */\n"
+ "  int i;\n"
+

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

I'm missing tests with both EmptyLineBeforeAccessModifier and 
EmptyLine(s)AfterAccessModifier options.
And possibly other options that could interfere with them.




Comment at: clang/include/clang/Format/Format.h:1957
+  /// Defines how many lines are put after access modifiers.
+  unsigned EmptyLinesAfterAccessModifier;
+

This option seems to be very different from `EmptyLineBeforeAccessModifier`. I 
don't mean in what it does, because this is analogical, but in the possible 
options.
Wouldn't it be less surprising to have (at least some) similar options here and 
there?
Is there any value in having more than one line after access modifiers? 
Couldn't that be achieved with Leave option?
How do the two options work together?

Also, the difference in singular vs. plural form of "Line(s)" in these two 
options is disconcerting.
From the user perspective, it's error-prone to have two options that are at the 
same time so similar and so different.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

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

HazardyKnusperkeks added a comment.

Please also add an entry in the `clang/doc/ReleaseNotes.rst`.




Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1284
   (!PreviousLine->InPPDirective || !RootToken.HasUnescapedNewline))
-Newlines = std::min(1u, Newlines);
+Newlines = Style.EmptyLinesAfterAccessModifier + 1u;
 

Max_S wrote:
> HazardyKnusperkeks wrote:
> > I don't know, I'm just asking:
> > Shouldn't this be `Newlines = std::min(Newlines, 
> > Style.EmptyLinesAfterAccessModifier + 1u);`?
> This is also possible but then the logic would be how many lines should be 
> kept at maximum after an access specifier.
> 
> The name would then be `Style.KeepMaximumLinesAfterAccessModifier`.
> 
> Currently the logic above:
> ```
>   if (Newlines == 0 && !RootToken.IsFirst)
> Newlines = 1;
> ```
> forces Newlines to be always 1 or bigger. Therefore the old logic would 
> always add one new line and I decided to implement the setting in the same 
> way.
With your explanation everything is fine here.



Comment at: clang/unittests/Format/FormatTest.cpp:9205
+   "};";
+  EXPECT_EQ(test1NL, format(test0NL));
+  verifyFormat(test1NL);

Max_S wrote:
> MyDeveloperDay wrote:
> > why can't you just verifyFormat them all?
> Yes. I will change this in the next update.
He `verifyFormat`s them with the right style, doesn't he?

With handling of empty lines I think it is useful to add the `EXPECT_EQ`.



Comment at: clang/unittests/Format/FormatTest.cpp:9212
+  StyleWithLine.EmptyLinesAfterAccessModifier = 1u;
+  EXPECT_EQ(test2NL, format(test0NL, StyleWithLine));
+  EXPECT_EQ(test2NL, format(test1NL, StyleWithLine));

Max_S wrote:
> MyDeveloperDay wrote:
> > yeah I'm not a fan of this like this... sorry... just write the test out in 
> > long form, when it goes wrong I don't have to be a compiler to understand 
> > what is going wrong I can just see it.
> I can change this, but the current output of the tests is (I forced the 
> error):
> ```
> //llvm-project/clang/unittests/Format/FormatTest.cpp:72: Failure
>   Expected: Expected.str()
>   Which is: "class Foo {\nprivate:\n\n  int i;\n};"
> To be equal to: format(Expected, Style)
>   Which is: "class Foo {\nprivate:\n  int i;\n};"
> With diff:
> @@ -1,5 @@
>  class Foo {
>  private:
> -
>int i;
>  };
> ```
> 
> Which is actually human readable in this case. Shall I still change it?
I'm a fan of it. :)
Especially with the demonstration that the string is still expanded.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Max Sagebaum via Phabricator via cfe-commits]

Max_S added a comment.

In D98237#2616193 , @MyDeveloperDay 
wrote:

> Just out of interest and we are supposed to ask for this but can you point to 
> public style guide that uses this style.   (actually I don't mind if other 
> formatting tools have this capability and you highlight it, like astyle or 
> editorConfig etc)
>
> From my perspective this seems like a reasonable suggestion. (even if I'm 
> unlikely to use it myself ;-))

I can not specify an other formater or a style guide that uses this style. The 
problem is that clang-format is rather destructive at this point. Other 
formatters like astyle or the one from CLion keep empty lines after an access 
modifier.  The behavior of clang-format before this patch was to remove all 
empty lines after an access modifier. The option `MaxEmptyLinesToKeep` is 
currently ignored at this point.

After thinking a little bit about this:
If the removal of all lines after an access modifier is rather a bug than a 
feature, then the change:

 if (PreviousLine && PreviousLine->First->isAccessSpecifier() &&
 (!PreviousLine->InPPDirective || !RootToken.HasUnescapedNewline))
  -Newlines = std::min(1u, Newlines);
  +Newlines = std::max(1u, Newlines);
  
 if (Newlines)

would solve this problem.




Comment at: clang/unittests/Format/FormatTest.cpp:9205
+   "};";
+  EXPECT_EQ(test1NL, format(test0NL));
+  verifyFormat(test1NL);

MyDeveloperDay wrote:
> why can't you just verifyFormat them all?
Yes. I will change this in the next update.



Comment at: clang/unittests/Format/FormatTest.cpp:9212
+  StyleWithLine.EmptyLinesAfterAccessModifier = 1u;
+  EXPECT_EQ(test2NL, format(test0NL, StyleWithLine));
+  EXPECT_EQ(test2NL, format(test1NL, StyleWithLine));

MyDeveloperDay wrote:
> yeah I'm not a fan of this like this... sorry... just write the test out in 
> long form, when it goes wrong I don't have to be a compiler to understand 
> what is going wrong I can just see it.
I can change this, but the current output of the tests is (I forced the error):
```
//llvm-project/clang/unittests/Format/FormatTest.cpp:72: Failure
  Expected: Expected.str()
  Which is: "class Foo {\nprivate:\n\n  int i;\n};"
To be equal to: format(Expected, Style)
  Which is: "class Foo {\nprivate:\n  int i;\n};"
With diff:
@@ -1,5 @@
 class Foo {
 private:
-
   int i;
 };
```

Which is actually human readable in this case. Shall I still change it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Just out of interest and we are supposed to ask for this but can you point to 
public style guide that uses this style.   (actually I don't mind if other 
formatting tools have this capability and you highlight it, like astyle or 
editorConfig etc)

From my perspective this seems like a reasonable suggestion. (even if I'm 
unlikely to use it myself ;-))




Comment at: clang/unittests/Format/FormatTest.cpp:9205
+   "};";
+  EXPECT_EQ(test1NL, format(test0NL));
+  verifyFormat(test1NL);

why can't you just verifyFormat them all?



Comment at: clang/unittests/Format/FormatTest.cpp:9212
+  StyleWithLine.EmptyLinesAfterAccessModifier = 1u;
+  EXPECT_EQ(test2NL, format(test0NL, StyleWithLine));
+  EXPECT_EQ(test2NL, format(test1NL, StyleWithLine));

yeah I'm not a fan of this like this... sorry... just write the test out in 
long form, when it goes wrong I don't have to be a compiler to understand what 
is going wrong I can just see it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Max Sagebaum via Phabricator via cfe-commits]

Max_S added inline comments.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1284
   (!PreviousLine->InPPDirective || !RootToken.HasUnescapedNewline))
-Newlines = std::min(1u, Newlines);
+Newlines = Style.EmptyLinesAfterAccessModifier + 1u;
 

HazardyKnusperkeks wrote:
> I don't know, I'm just asking:
> Shouldn't this be `Newlines = std::min(Newlines, 
> Style.EmptyLinesAfterAccessModifier + 1u);`?
This is also possible but then the logic would be how many lines should be kept 
at maximum after an access specifier.

The name would then be `Style.KeepMaximumLinesAfterAccessModifier`.

Currently the logic above:
```
  if (Newlines == 0 && !RootToken.IsFirst)
Newlines = 1;
```
forces Newlines to be always 1 or bigger. Therefore the old logic would always 
add one new line and I decided to implement the setting in the same way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Max Sagebaum via Phabricator via cfe-commits]

Max_S added a comment.

In D98237#2614844 , 
@HazardyKnusperkeks wrote:

> I would like additional tests:
>
> - With at least 2 empty lines
> - With 0-2 empty lines without `verifyFormat` to demonstrate what is kept, 
> added, and removed.

Thank you for reviewing this, I added new tests, that demonstrate the 
formatting behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

[Max Sagebaum via Phabricator via cfe-commits]

Max_S updated this revision to Diff 329574.
Max_S added a comment.

Updating D98237 : [clang-format] Option for 
empty lines after an access modifier.

- Added additional verification tests
- Added expected formating tests
- Changed comment to reflect the new logic


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -9179,6 +9179,48 @@
Style);
 }
 
+TEST_F(FormatTest, FormatsAfterAccessModifiers) {
+  char const* const test0NL =
+   "class Foo {\n"
+   "private: int i;\n"
+   "};";
+  char const* const test1NL =
+   "class Foo {\n"
+   "private:\n"
+   "  int i;\n"
+   "};";
+  char const* const test2NL =
+   "class Foo {\n"
+   "private:\n"
+   "\n"
+   "  int i;\n"
+   "};";
+  char const* const test3NL =
+   "class Foo {\n"
+   "private:\n"
+   "\n"
+   "\n"
+   "  int i;\n"
+   "};";
+  EXPECT_EQ(test1NL, format(test0NL));
+  verifyFormat(test1NL);
+  EXPECT_EQ(test1NL, format(test2NL));
+  EXPECT_EQ(test1NL, format(test3NL));
+
+  FormatStyle StyleWithLine = getLLVMStyle();
+  StyleWithLine.EmptyLinesAfterAccessModifier = 1u;
+  EXPECT_EQ(test2NL, format(test0NL, StyleWithLine));
+  EXPECT_EQ(test2NL, format(test1NL, StyleWithLine));
+  verifyFormat(test2NL, StyleWithLine);
+  EXPECT_EQ(test2NL, format(test3NL, StyleWithLine));
+
+  StyleWithLine.EmptyLinesAfterAccessModifier = 2u;
+  EXPECT_EQ(test3NL, format(test0NL, StyleWithLine));
+  EXPECT_EQ(test3NL, format(test1NL, StyleWithLine));
+  EXPECT_EQ(test3NL, format(test2NL, StyleWithLine));
+  verifyFormat(test3NL, StyleWithLine);
+}
+
 TEST_F(FormatTest, FormatsArrays) {
   verifyFormat("a[a]\n"
" [b] = c;");
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -1278,10 +1278,10 @@
 }
   }
 
-  // Remove empty lines after access specifiers.
+  // Force the configured amount of new lines after an access specifier
   if (PreviousLine && PreviousLine->First->isAccessSpecifier() &&
   (!PreviousLine->InPPDirective || !RootToken.HasUnescapedNewline))
-Newlines = std::min(1u, Newlines);
+Newlines = Style.EmptyLinesAfterAccessModifier + 1u;
 
   if (Newlines)
 Indent = NewlineIndent;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -586,6 +586,8 @@
 IO.mapOptional("DisableFormat", Style.DisableFormat);
 IO.mapOptional("EmptyLineBeforeAccessModifier",
Style.EmptyLineBeforeAccessModifier);
+IO.mapOptional("EmptyLinesAfterAccessModifier",
+   Style.EmptyLinesAfterAccessModifier);
 IO.mapOptional("ExperimentalAutoDetectBinPacking",
Style.ExperimentalAutoDetectBinPacking);
 IO.mapOptional("FixNamespaceComments", Style.FixNamespaceComments);
@@ -975,6 +977,7 @@
   LLVMStyle.DeriveLineEnding = true;
   LLVMStyle.DerivePointerAlignment = false;
   LLVMStyle.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_LogicalBlock;
+  LLVMStyle.EmptyLinesAfterAccessModifier = 0u;
   LLVMStyle.ExperimentalAutoDetectBinPacking = false;
   LLVMStyle.FixNamespaceComments = true;
   LLVMStyle.ForEachMacros.push_back("foreach");
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -1953,6 +1953,9 @@
   /// Defines in which cases to put empty line before access modifiers.
   EmptyLineBeforeAccessModifierStyle EmptyLineBeforeAccessModifier;
 
+  /// Defines how many lines are put after access modifiers.
+  unsigned EmptyLinesAfterAccessModifier;
+
   /// If ``true``, clang-format detects whether function calls and
   /// definitions are formatted with one parameter per line.
   ///
@@ -3201,6 +3204,7 @@
DerivePointerAlignment == R.DerivePointerAlignment &&
DisableFormat == R.DisableFormat &&
EmptyLineBeforeAccessModifier == R.EmptyLineBeforeAccessModifier &&
+   EmptyLines

[PATCH] D98237: [clang-format] Option for empty lines after an access modifier.

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

HazardyKnusperkeks added a comment.

I would like additional tests:

- With at least 2 empty lines
- With 0-2 empty lines without `verifyFormat` to demonstrate what is kept, 
added, and removed.




Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1284
   (!PreviousLine->InPPDirective || !RootToken.HasUnescapedNewline))
-Newlines = std::min(1u, Newlines);
+Newlines = Style.EmptyLinesAfterAccessModifier + 1u;
 

I don't know, I'm just asking:
Shouldn't this be `Newlines = std::min(Newlines, 
Style.EmptyLinesAfterAccessModifier + 1u);`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98237

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