[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