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

Mon Mar 22 03:40:40 PDT 2021

Max_S marked 4 inline comments as done.
Max_S added a comment.

> 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?

In D98237#2637832 <https://reviews.llvm.org/D98237#2637832>, @MyDeveloperDay wrote:

> 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?



================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2159
+    Always add empty line after access modifiers if there are none.
+    MaxEmptyLinesToKeep is applied also.
+
----------------
MyDeveloperDay wrote:
> 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.
> 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. 


================
Comment at: clang/unittests/Format/FormatTest.cpp:9392
+                                     "};\n";
+  EXPECT_EQ(NL_B_3_A_1_I_3, format(NL_B_3_A_3_I_3, Style));
+
----------------
MyDeveloperDay wrote:
> 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
> 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.


================
Comment at: clang/unittests/Format/FormatTest.cpp:9212
+  StyleWithLine.EmptyLinesAfterAccessModifier = 1u;
+  EXPECT_EQ(test2NL, format(test0NL, StyleWithLine));
+  EXPECT_EQ(test2NL, format(test1NL, StyleWithLine));
----------------
MyDeveloperDay wrote:
> Max_S wrote:
> > 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;
> >  };
> > 
> > ````
> why would we break with convention?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98237/new/

https://reviews.llvm.org/D98237

