[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):
> ```
> /<path>/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