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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
