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