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