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
