Page MenuHomePhabricator

[clang-format] Option for empty lines after an access modifier.
ClosedPublic

Authored by Max_S on Mar 8 2021, 11:49 PM.

Details

Summary

The current logic for access modifiers in classes ignores the option 'MaxEmptyLinesToKeep=1'. It is therefore impossible to have a coding style that requests one empty line after an access modifier. The patch allows the user to configure how many empty lines clang-format should add after an access modifier. This will remove lines if there are to many and will add them if there are missing.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Max_S requested review of this revision.Mar 8 2021, 11:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2021, 11:49 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
lebedev.ri retitled this revision from Option for empty lines after an access modifier. to [clang-format] Option for empty lines after an access modifier..Mar 8 2021, 11:54 PM
Eugene.Zelenko added a project: Restricted Project.

I would like additional tests:

  • With at least 2 empty lines
  • With 0-2 empty lines without verifyFormat to demonstrate what is kept, added, and removed.
clang/lib/Format/UnwrappedLineFormatter.cpp
1285–1304

I don't know, I'm just asking:
Shouldn't this be Newlines = std::min(Newlines, Style.EmptyLinesAfterAccessModifier + 1u);?

Max_S updated this revision to Diff 329574.Mar 10 2021, 1:58 AM

Updating D98237: [clang-format] Option for empty lines after an access modifier.

  • Added additional verification tests
  • Added expected formating tests
  • Changed comment to reflect the new logic
Max_S added a comment.Mar 10 2021, 2:01 AM

I would like additional tests:

  • With at least 2 empty lines
  • With 0-2 empty lines without verifyFormat to demonstrate what is kept, added, and removed.

Thank you for reviewing this, I added new tests, that demonstrate the formatting behavior.

Max_S added inline comments.Mar 10 2021, 2:11 AM
clang/lib/Format/UnwrappedLineFormatter.cpp
1285–1304

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.

Just out of interest and we are supposed to ask for this but can you point to public style guide that uses this style. (actually I don't mind if other formatting tools have this capability and you highlight it, like astyle or editorConfig etc)

From my perspective this seems like a reasonable suggestion. (even if I'm unlikely to use it myself ;-))

clang/unittests/Format/FormatTest.cpp
9205

why can't you just verifyFormat them all?

9212

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.

Max_S added a comment.EditedMar 10 2021, 5:52 AM

Just out of interest and we are supposed to ask for this but can you point to public style guide that uses this style. (actually I don't mind if other formatting tools have this capability and you highlight it, like astyle or editorConfig etc)

From my perspective this seems like a reasonable suggestion. (even if I'm unlikely to use it myself ;-))

I can not specify an other formater or a style guide that uses this style. The problem is that clang-format is rather destructive at this point. Other formatters like astyle or the one from CLion keep empty lines after an access modifier. The behavior of clang-format before this patch was to remove all empty lines after an access modifier. The option MaxEmptyLinesToKeep is currently ignored at this point.

After thinking a little bit about this:
If the removal of all lines after an access modifier is rather a bug than a feature, then the change:

   if (PreviousLine && PreviousLine->First->isAccessSpecifier() &&
       (!PreviousLine->InPPDirective || !RootToken.HasUnescapedNewline))
-    Newlines = std::min(1u, Newlines);
+    Newlines = std::max(1u, Newlines);

   if (Newlines)

would solve this problem.

The format test suite runs with this patch, the only error is:

[ RUN      ] FormatTest.SeparatesLogicalBlocks
/home/msagebaum/Kaiserslautern/Programms/temp/llvm-project/clang/unittests/Format/FormatTest.cpp:1852: Failure
      Expected: "class A {\n" "protected:\n" "public:\n" "  void f();\n" "};"
      Which is: "class A {\nprotected:\npublic:\n  void f();\n};"
To be equal to: format("class A {\n" "protected:\n" "\n" "public:\n" "\n" "  void f();\n" "};")
      Which is: "class A {\nprotected:\n\npublic:\n\n  void f();\n};"
With diff:
@@ +1,7 @@
 class A {
 protected:
+
 public:
+
   void f();
 };

/home/msagebaum/Kaiserslautern/Programms/temp/llvm-project/clang/unittests/Format/FormatTest.cpp:1866: Failure
      Expected: "#define B     \\\n" "  class A {   \\\n" "   protected: \\\n" "   public:    \\\n" "    void f(); \\\n" "  };"
      Which is: "#define B     \\\n  class A {   \\\n   protected: \\\n   public:    \\\n    void f(); \\\n  };"
To be equal to: format("#define B     \\\n" "  class A {   \\\n" "   protected: \\\n" "              \\\n" "   public:    \\\n" "              \\\n" "    void f(); \\\n" "  };", getGoogleStyle())
      Which is: "#define B     \\\n  class A {   \\\n   protected: \\\n              \\\n   public:    \\\n              \\\n    void f(); \\\n  };"
With diff:
@@ +2,7 @@
   class A {   \\
    protected: \\
+              \\
    public:    \\
+              \\
     void f(); \\
   };

[  FAILED  ] FormatTest.SeparatesLogicalBlocks (7 ms)
clang/unittests/Format/FormatTest.cpp
9205

Yes. I will change this in the next update.

9212

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?

Please also add an entry in the clang/doc/ReleaseNotes.rst.

clang/lib/Format/UnwrappedLineFormatter.cpp
1285–1304

With your explanation everything is fine here.

clang/unittests/Format/FormatTest.cpp
9205

He verifyFormats them with the right style, doesn't he?

With handling of empty lines I think it is useful to add the EXPECT_EQ.

9212

I'm a fan of it. :)
Especially with the demonstration that the string is still expanded.

I'm missing tests with both EmptyLineBeforeAccessModifier and EmptyLine(s)AfterAccessModifier options.
And possibly other options that could interfere with them.

clang/include/clang/Format/Format.h
2005

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.

curdeius requested changes to this revision.Mar 12 2021, 1:04 PM
This revision now requires changes to proceed.Mar 12 2021, 1:04 PM
Max_S updated this revision to Diff 330588.Mar 15 2021, 3:00 AM

Changed the option to EmptyLineAfterAccessModifier and allowed the options Never, Leave and Always. Updated the tests accordingly and also added an entry in the changelog.

Max_S added a comment.Mar 15 2021, 3:11 AM

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.

clang/include/clang/Format/Format.h
2005

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.

clang/unittests/Format/FormatTest.cpp
9212

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;
 };

`

That looks really good. Please add tests as indicated in the inline comments and fix the formatting (see clang-format warnings).

clang/include/clang/Format/Format.h
1912

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.

2005

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));
clang/include/clang/Format/Format.h
1914

It does not always add, it does enforces one empty line, or am I mistaken?

Max_S updated this revision to Diff 331818.Mar 19 2021, 3:37 AM

I added more tests for the interaction between MaxEmptyLinesToKeep, EmptyLineBeforeAccessModifier and EmptyLineAfterAccessModifier. During these tests I recognized, that EmptyLineBeforeAccessModifier = Always,Leave will (should adhere) to MaxEmptyLinesToKeep. So the option does not force the new line it only applies it if it is missing. I changed the logic for EmptyLineAfterAccessModifier accordingly.

Four of the new tests fail, two of them are probably bugs in the implementation of EmptyLineBeforeAccessModifier.

The test in line 9354 is set to:

Style.MaxEmptyLinesToKeep = 0u; // Both force one new line
Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Always;
Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Always;
EXPECT_EQ(NL_B_1_A_1_I_1, format(NL_B_3_A_3_I_3, Style));

So both options should force here the newline, that are removed by MaxEmptyLinesToKeep. EmptyLineBeforeAccessModifier removes all lines, so it seems that MaxEmptyLinesToKeep has the higher rank here. Is that correct behavior?

The test in line 9368 is set to:

Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Leave;
Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Leave;
Style.MaxEmptyLinesToKeep = 1u; 
EXPECT_EQ(NL_B_1_A_1_I_1, format(NL_B_3_A_3_I_3, Style));

So both options should leave the new lines in place but should adhere to MaxEmptyLinesToKeep. EmptyLineBeforeAccessModifier keeps all three lines, so it seems that MaxEmptyLinesToKeep has the lower rank here. Is that correct behavior?

The tests in line 9462 and 9466 require some additional implementation. EmptyLineAfterAccessModifier is adding the three lines since it is configured to do so, but EmptyLineBeforeAccessModifier would remove these lines. Since EmptyLineBeforeAccessModifier can only access the old file and not the new one. It does not know that three lines have been written and probably could not remove them.

I would like to implement it in such a way, that EmptyLineAfterAccessModifier looks ahead and skips its logic if the next token is a also an access modifier such that the logic of EmptyLineBeforeAccessModifier takes precedence. I could not find a way to get the next line. Is this possible somehow? Does there exist some helper functionality?

Max_S added inline comments.Mar 19 2021, 3:43 AM
clang/include/clang/Format/Format.h
1914

It adds it if it is missing, but leaves additional lines. All in all MaxEmptyLinesToKeep has precedence here. If MaxEmptyLinesToKeep is zero, then no line is added.

MyDeveloperDay added inline comments.Mar 19 2021, 9:49 AM
clang/unittests/Format/FormatTest.cpp
9212

why would we break with convention?

MyDeveloperDay requested changes to this revision.Mar 19 2021, 9:54 AM
MyDeveloperDay added inline comments.
clang/unittests/Format/FormatTest.cpp
9392

I can't read this, I can't tell if you've fat fingered a test, I feel these are better done systematically one by one with the text in the verifyFormat("...") it makes it easier when they fail to understand what is going on.

Also its ok to have more than one TEST_F if you want to handle the different states in small batches

I think you undetersand what NL_B_3_A_0_I_0 means and how it differs form NL_B_0_A_3_I_0 but others won't

This revision now requires changes to proceed.Mar 19 2021, 9:54 AM

could you please mark your comments done when they are done.

clang/docs/ClangFormatStyleOptions.rst
2132
2159

if MaxEmptyLineToKeep is 0 then surely ELAAMS_Always should win, as this is the lesser i.e. if people never uses extra lines except after access modifiers this is what they would set? did I missunderstand?

otherwise doesn't MaxEmptyLinesToKeep just nullify the whole option? meaning its useless for anyone using it?

I agree in the ELAAMS_Leave case that MaxEmptyLinesToKeep should win.

Max_S updated this revision to Diff 332236.Mar 22 2021, 3:30 AM

Fixed interaction between Before and After configurations options. Before handles the case
of two access modifiers specification following each other.

Max_S marked 4 inline comments as done.Mar 22 2021, 3:40 AM

The tests in line 9462 and 9466 require some additional implementation. EmptyLineAfterAccessModifier is adding the three lines since it is configured to do so, but EmptyLineBeforeAccessModifier would remove these lines. Since EmptyLineBeforeAccessModifier can only access the old file and not the new one. It does not know that three lines have been written and probably could not remove them.

I would like to implement it in such a way, that EmptyLineAfterAccessModifier looks ahead and skips its logic if the next token is a also an access modifier such that the logic of EmptyLineBeforeAccessModifier takes precedence. I could not find a way to get the next line. Is this possible somehow? Does there exist some helper functionality?

I fixed this in the newest change set. EmptyLineBeforeAccessModifier handles the case when two access modifiers follow each other. I updated the documentation accordingly.

Four of the new tests fail, two of them are probably bugs in the implementation of EmptyLineBeforeAccessModifier.

The test in line 9354 is set to:

Style.MaxEmptyLinesToKeep = 0u; // Both force one new line
Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Always;
Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Always;
EXPECT_EQ(NL_B_1_A_1_I_1, format(NL_B_3_A_3_I_3, Style));

So both options should force here the newline, that are removed by MaxEmptyLinesToKeep. EmptyLineBeforeAccessModifier removes all lines, so it seems that MaxEmptyLinesToKeep has the higher rank here. Is that correct behavior?

The test in line 9368 is set to:

Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_Leave;
Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Leave;
Style.MaxEmptyLinesToKeep = 1u; 
EXPECT_EQ(NL_B_1_A_1_I_1, format(NL_B_3_A_3_I_3, Style));

So both options should leave the new lines in place but should adhere to MaxEmptyLinesToKeep. EmptyLineBeforeAccessModifier keeps all three lines, so it seems that MaxEmptyLinesToKeep has the lower rank here. Is that correct behavior?

I still need some advice on how to handle this. Should I fix the test so that it captures the current behavior. Should I fix the logic or should I open a bug report?

could you please mark your comments done when they are done.

I usually try to do this. How can I see which ones are still open?

clang/docs/ClangFormatStyleOptions.rst
2159

if MaxEmptyLineToKeep is 0 then surely ELAAMS_Always should win, as this is the lesser i.e. if people never uses extra lines except after access modifiers this is what they would set? did I missunderstand?

You understood correctly and that is the way it is implemented.

I agree in the ELAAMS_Leave case that MaxEmptyLinesToKeep should win.

That is also the way it is implemented.

I fixed the problem with two access modifiers in a row.

clang/unittests/Format/FormatTest.cpp
9212

verifyFormat errors look like:

/<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;
 };

EXPECT_EQ errors look like:

/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;
 };

In the first approach you do not get the actual line which the error created (just the line in a helper function). The stacktrace afterwards tells you wehre the error origniated.
I can change everything to verifyFormat, personally I think that EXPECT_EQ has the improved output.

Please give me a go / no go to change everything to verifyFormat.

9392

I can't read this,

I may have missed here a comment. The convention is:

NewLines_Before_Access_Modifier_3_After_Access_Modifier_3_Inbetween_Access_Modifier_3
N     L_       B_                                       3_A_                                    3_I_                                               3
NL_B_3_A_3_I_3

I can't tell if you've fat fingered a test

What do you mean by that?

I feel these are better done systematically

Thats what I have actually done. First tests only concerning EmptyLineAfterAccessModifier afterwards the combinations with EmptyLineAfterAccessModifier and EmptyLineBeforeAccessModifier.
You have the matrix:

Before\After | Never | Leave | Always
Never        |     x     |    x      |     x
Leave        |     x     |    x      |     x
Always       |     x     |    x      |     x
LogicalBlock |     x     |    x      |     x

with the text in the verifyFormat("...") it makes it easier when they fail to understand what is going on.

I do not think that verifyFormat is the better option. Both provide you with the actual string and the diff. verifyFormat gives you some internal variable names, EXPECT_EQ the names or strings you have provided which is IMHO more readable.

As mentioned in the comment to line 9207 I can gladly change this if requested.
If I expand the string the test will expand from 289 lines to approx. 500 lines.

Also its ok to have more than one TEST_F if you want to handle the different states in small batches

I will split it into two tests on the next change. One that tests EmptyLineAfterAccessModifier and one that tests the interaction EmptyLineAfterAccessModifier and EmptyLineBeforeAccessModifier.

For the interaction I can reduce it to a smaller code sample, that just has the interaction. That is:

struct foo {
private:



protected:
};

Please give me a go / no go on:

  • Adding comment for naming convention
  • A more systematic approach (please specify what you have in your mind, what would be a better approach.)
  • Change everything to verifyFormat
  • Remove the variables and expand everything to the arguments
  • Reduce the interaction test to a smaller code sample. (This would also change the naming convention to NL_Between_3.)

I will defenetly do:

  • Split it into two tests.

If you follow people tweeting about clang-format (as I do) and you look through the bug tracking system, one major criticism of clang-format is that the second clang-format can be different from the first, sometimes an equilibrium can be found sometimes not.

When I started working on clang-format I was encouraged to use verifyFormat() as it tests that scenario and also tries to mess up the format and ensure it returns to the desired state. It found bugs in my code which would have made clang-format worse.

Apart from it being the convention I believe it makes for much more readable code, even if there is repetition as I don't need to keep cross referencing variables with rather obscure names NL_B_3_A_0_I_0 this is unnecessary noise and makes the code overly verbose.

No you'll need to check out what the messUp() function is actually doing but I think by and large IMHO we should stick with verifyFormat.

I'd be quite interested to understand what the impact (if any) would be on javascript and C# formatting

Max_S added a comment.Mar 23 2021, 8:03 AM

If you follow people tweeting about clang-format (as I do) and you look through the bug tracking system, one major criticism of clang-format is that the second clang-format can be different from the first, sometimes an equilibrium can be found sometimes not.

When I started working on clang-format I was encouraged to use verifyFormat() as it tests that scenario and also tries to mess up the format and ensure it returns to the desired state. It found bugs in my code which would have made clang-format worse.

Apart from it being the convention I believe it makes for much more readable code, even if there is repetition as I don't need to keep cross referencing variables with rather obscure names NL_B_3_A_0_I_0 this is unnecessary noise and makes the code overly verbose.

No you'll need to check out what the messUp() function is actually doing but I think by and large IMHO we should stick with verifyFormat.

Ok then I will change the tests accordingly. This reasoning should be written down somewhere.

I'd be quite interested to understand what the impact (if any) would be on javascript and C# formatting

I have not done anything in javascript a quick google search showed no such modifiers in classes. Do you have an example? C# seems to handle it like java and then the modifiers become properties of the functions member, there should be no influence.

Max_S updated this revision to Diff 334186.Mar 30 2021, 9:16 AM

Cleanup of tests and additonal logic for modifiers at the end of a class.

Max_S marked 9 inline comments as done.Mar 30 2021, 9:44 AM

In the last update I fixed now everything and changed the tests.

The tests use now verify format when possible. A bug here, kept me from doing it in a few places. I submitted a bug report for this: https://bugs.llvm.org/show_bug.cgi?id=49772

I hope this is now fine and can be merged into the master.

clang/include/clang/Format/Format.h
1912

I submitted a bug report about this, since the behavior is different.

https://bugs.llvm.org/show_bug.cgi?id=49757

MyDeveloperDay accepted this revision.Mar 31 2021, 7:59 AM

LGTM thank you for trying where you could to use verifyFormat

curdeius accepted this revision.Mar 31 2021, 8:09 AM

LGTM, but please clean up the comments before landing.

clang/docs/ClangFormatStyleOptions.rst
2132

Please apply this when landing.

clang/unittests/Format/FormatTest.cpp
9289

Nit. Here and elsewhere.

9546

Please rephrase/move the comment to be clear that it concerns ELBAMS/ELAAMS.

9598

Nit.

9605

Please end comments with full stops. Here and elsewhere.

This revision is now accepted and ready to land.Mar 31 2021, 8:09 AM
Max_S updated this revision to Diff 334523.EditedMar 31 2021, 1:43 PM
Max_S marked 7 inline comments as done.

Thank you for the reviews.

I addressed last minor remarks.

Do I need to worry about the build failures. (I do not think, that they are related to the changes.)

Do I need to do something to start the final merge?

Max_S added a comment.Apr 15 2021, 2:00 AM

Just wanted to ask if there is something missing for the merge?

I think you can land it. The issues seem indeed unrelated.
You might want to rebase and retest before landing to be sure.

Max_S added a comment.Apr 15 2021, 3:22 AM

Thank you for the answer. It did not know that I have to land it myself.

Now I read up on https://llvm.org/docs/Phabricator.html

Tried to land via 'arc land' but I do not have the access rights.

So: Can someone please land the change for me. :)

I'll land it for you. Could you please provide "Name Surname <email@org>" for commit attribution unless the info associated with your phabricator user is okay?

Max_S added a comment.Apr 15 2021, 3:55 AM

Thank you for landing it. The information should be ok. But anyway: Max Sagebaum <max.sagebaum@scicomp.uni-kl.de>

This revision was landed with ongoing or failed builds.Apr 15 2021, 12:03 PM
This revision was automatically updated to reflect the committed changes.