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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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: |
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
Thank you for reviewing this, I added new tests, that demonstrate the formatting behavior.
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. |
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. :) |
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. Also, the difference in singular vs. plural form of "Line(s)" in these two options is disconcerting. |
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.
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? | |
2005 | I like the new version. Thank you for the update. 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? |
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?
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. |
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
9212 | why would we break with convention? |
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 |
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. |
Fixed interaction between Before and After configurations options. Before handles the case
of two access modifiers specification following each other.
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?
I usually try to do this. How can I see which ones are still open?
clang/docs/ClangFormatStyleOptions.rst | ||
---|---|---|
2159 |
You understood correctly and that is the way it is implemented.
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. Please give me a go / no go to change everything to verifyFormat. | |
9392 |
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
What do you mean by that?
Thats what I have actually done. First tests only concerning EmptyLineAfterAccessModifier afterwards the combinations with EmptyLineAfterAccessModifier and EmptyLineBeforeAccessModifier. Before\After | Never | Leave | Always Never | x | x | x Leave | x | x | x Always | x | x | x LogicalBlock | x | x | x
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.
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:
I will defenetly do:
|
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
Ok then I will change the tests accordingly. This reasoning should be written down somewhere.
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.
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. |
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. |
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?
I think you can land it. The issues seem indeed unrelated.
You might want to rebase and retest before landing to be sure.
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?
Thank you for landing it. The information should be ok. But anyway: Max Sagebaum <max.sagebaum@scicomp.uni-kl.de>
When you made this commit you didn't regenerate the ClangFormatStyleOptions.rst from the Format.h using clang/docs/tool/dump_format_style.py now anyone else using the tool hit issues with code which is incorrect, we need to update Format.h to match what you expect in ClangFormatStyleOptions.rst
I am sorry about this. I provided a bugfix in D102989, can you please have a look if this is enough?
Is there a documentation I forget to read?