This adds a new formatter to arrange array of struct initializers into neat columns
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
16425 | What's missing is to format your last code with a column limit of 20 or so. Regards using EXPECT_EQ: EXPECT_EQ("test demo[] = {\n" " {56, 23, \"hello world i am a very long line that really, in any " "\"\n" " \"just world, ought be split over multiple lines\" },\n" " {-1, 93463, \"world\" },\n" " {7, 5, \"!!\" },\n" "};\n", format("test demo[] = {{56, 23, \"hello world i am a very long line that really, in any just world, ought be split over multiple lines\"},{-1, 93463, \"world\"},{7, 5, \"!!\"},};", Style)); This should be done with ColumnLimit 0, 20, 100. With verifyFormat you verify that the formatting is stable, but especially with no column limit there is often no stable. |
clang/docs/ReleaseNotes.rst | ||
---|---|---|
256–257 | ||
clang/lib/Format/UnwrappedLineFormatter.cpp | ||
920–923 ↗ | (On Diff #343534) | Having this in a function seems like a bad idea, it just makes the code less readable. Please inline. |
clang/unittests/Format/FormatTest.cpp | ||
16404 | Personally, I'm not really fond of testing all the combinations of styles. It adds little value to those tests. | |
16477 | Please add a test with preprocessor directives, e.g.: verifyFormat("struct test demo[] = {\n" " {56, 23, \"hello\" },\n" "#if X\n" " {-1, 93463, \"world\" },\n" "#endif\n" " {7, 5, \"!!\" }\n" "};\n", Style); | |
16485 | Nit: "ought to be" :). |
I think I have all the tests and requested review comments rolled up
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
16404 | I think it could be constexpr (thats the direction I originally started which lead to much moaning about the lack of fold expressions) however getLLVMStyle() is not constexpr and it felt inappropriate for this commit to change the interface to that function even if the change were relatively harmless. |
Something which just came to my mind. Since you wrote your own LineFormatter, you have to add test cases for all kinds of indention and wrapping. It seems that the string wrapping does not happen, how about comments? Are classes, namespaces, functions, etc. correctly indented with various configurations?
Given my expectations I think the alignment has to happen differently, but I'm happy to be proven wrong.
I still need to handle the 20 character case appropriately
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
16404 | I can certainly strip it down to corner cases. I guess I am not clear on which ones I should focus on. When I implemented this none of the combinations created problems. I could pick a few at random? But that doesn't feel like an improvement. | |
16517 | It should. When I use the command line invocation I get different output #include <array> struct test { int a, b; const char *c; }; int main( int, const char **) { std::array< struct test, 3> demo; demo = std::array< struct test, 3>{ test{ 56, 23, "hello " "world i " "am a " "very " "long " "line " "that " "really, " "in any " "just " "world, " "ought " "to be " "split " "over " "multiple" " lines"}, test{-1, 93463, "world"}, test{7, 5, "!!"}, }; } I will investigate |
hmmmmmm I think you are correct. Or at least the path I am going down with getting the case of a 20 character column width is demanding adding a bunch of things that replicate what is already there. Does a more fruitful avenue look something like annotating the tokens?
This looks like a good start..
All your tests are 3x3 have you considered mixing it up a bit. i.e. 2x3, what is the impact on 1x5 and 5x1 ?
Also how about nested structs, I'm interested to see what happens
{56, 23, { "ABC", 35 }} {57, 24, { "DEF", 36 }}
Thanks. I am reworking it so I handle line breaking in a sane fashion and dropping the LineFormatter override since that really can't handle reformatting without reimplementing most of it
All your tests are 3x3 have you considered mixing it up a bit. i.e. 2x3, what is the impact on 1x5 and 5x1 ?
I am also adding a bunch more tests. I'll roll this in.
Also how about nested structs, I'm interested to see what happens
{56, 23, { "ABC", 35 }} {57, 24, { "DEF", 36 }}
Will do. I added the braced initializer int constructor test to exercise the same code path. But it can't hurt to get this in there
This is a valiant effort, and I definitely don't want to discourage..especially as you shown a good understand of clang-format. Its been a long asked for feature.. but we should really ensure this covers the bulk of the complaints if possible..
For me I use this.. a search for clang-format off in github projects
https://github.com/search?q=clang-format+off
Status AbsGrad(const AttrSlice& attrs, FunctionDef* g) { // clang-format off return GradForUnaryCwise(g, { {{"sign"}, "Sign", {"x"}, {}, {"dy"}}, {{"dx"}, "Mul", {"dy", "sign"}}, }); // clang-format on }
it would be my ideal if we could irradiate the need for these, as they heavily dominate.
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
16441 | may be worth testing a case with comments at the end of each line verifyFormat("struct test demo[3] = {\n" " {56, 23, \"hello\" }, // first line\n" " {-1, 93463, \"world\" }, /* second line */\n" " {7, 5, \"!!\" } // at the end\n" "};\n", Style); |
This reworks substantially this commit. I recognize there are lacking/broken
tests but I just would like to ensure that the general direction doesn't
seem likely to end in tears
I'm not deep enough in clang-format and currently have not enough time to check that in depth. But why are you right aligning?
I would love to have a right aligning for numbers (or even better aligning around the decimal dot) in all my code, but strings I wouldn't want to right align.
So the basic approach here is to add the column aligning at the point that the lines are broken to make sure that we can align to the indented, now broken, columns. The right alignment was the easier path (at the moment) since the spaces attached to tokens in the change list proceeded the token and since I was calculating the column size with spaces based on a pointer attached to the token in the same column. To left align at each column I'd need to look at the adjacent column to determine the right number of spaces.
I would love to have a right aligning for numbers (or even better aligning around the decimal dot) in all my code, but strings I wouldn't want to right align.
I think we could certainly do that. I guess at this point (and this is somewhat motivated by the fact that I don't, and probably shouldn't, have commit rights the to llvm repo) I think if we want to move this commit forward we ought to agree on a set of changes and subsequent tests that we can all be comfortable with that would make this a viable bit of code. I don't think it has deep problems in the sense the prior one did and so should be amenable to laundry listing what we need and I'll move it forward. I think this set of tests should be added/fixed:
A test where the line is broken in the middle and/or first column (line breaking is really the sticky bit)
Fixing the test where the 100 column limit doesn't format correctly
Adding a test with several arrays to format and varying the other alignment options to make sure that doesn't confuse anything
I guess a final question I have would be, if we get this list sorted who can/would be capable of committing this change to the repo?
As said, I would really love more advanced alignment options, but I think we should keep to what clang-format does now, left aligning.
The tests seem to be reasonable, a combination of previous (mis)alignment and comments (end of line and in the middle) should be added.
clang/lib/Format/TokenAnnotator.cpp | ||
---|---|---|
2650 | I don't know if there are already NOLINTs in the code and if there should be, on a different change I saw there are some messages from clang-tidy in the existing code which are ignored (not NOLINTed). That's something maybe someone with more experience can answer. | |
2662 | Although I'm not a big fan of it, I think the LLVM style is no braces for one line if and else. | |
2681 | This one I'm against, just do what clang-tidy says remove the else. (Others may disagree.) |
I picked up most of these. One of the tests is already covered (I think) maybe I am misunderstanding
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
16512 | I have this check up on lines 16384-16397 |
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
16512 | Sorry for that, I just looked at the diff between the revisions. I perform full review only if we are near the final decision and don't have all things (especially all test cases) in memory. |
I am of the view that this is complete. Or at a minimum all the tests that have
been requested pass and I would be surprised if there were gross errors or other
major things I have missed
I've added a few comments, and I would like to hear the opinion of others regarding the left or right alignment of the elements.
clang/lib/Format/ContinuationIndenter.cpp | ||
---|---|---|
603 | Please don't remove the empty lines. | |
clang/lib/Format/TokenAnnotator.cpp | ||
733 | Capital letter for variables. | |
733 | Where does the 2 come from? How do we know that + 2 is always applicable? There should be an explanation, and an assert. On another note, I would prefer std::next(). | |
2657–2659 | There are still braces. | |
2663 | I think if you revert the condition and drop the else (because of the break) makes it more readable. | |
clang/lib/Format/TokenAnnotator.h | ||
35 | Add a trailing comma, so that future additions do not need to change this line. | |
clang/lib/Format/WhitespaceManager.h | ||
254 | For iterators prefix increment is really better than postfix. |
Remains the issue with the alignment, I would like to know @MyDeveloperDay 's opinion on that. Should the values be right aligned, or left aligned? As far as I see all alignment in clang-format is left until now.
clang/lib/Format/WhitespaceManager.cpp | ||
---|---|---|
974 | This is unneccessary. (But doesn't need a new revision until approved.) | |
1073 | Same |
I think whatever you choose someone will ask for the opposite, maybe consider making it an option now while you are in the code and understand it.
Ok. Given @HazardyKnusperkeks comment I'll make the default be left alignment and update the tests accordingly
You could add AlignArrayOfStructuresMemberAlignment Left:Right you have the code to right align, your about to change it to left align, seems like you've done all the work.
Better still how about having
ArraryMemberAligmentEnum AlignArrayOfStructures
of "None, Left and Right"
I find boolean options don't stay boolean for long!
Yeah once I started doing the work this is exactly the path I am going down. Save the name. I should have it rolled up shortly.
Save the name. I should have it rolled up shortly.
You an keep the name, just change the type to be an enum
clang/docs/ClangFormatStyleOptions.rst | ||
---|---|---|
216–221 | Don't forget to re-generate .rst. |
Got some changes. The failing test is an open issue
clang/docs/ClangFormatStyleOptions.rst | ||
---|---|---|
216–221 | I am sorry. I am not entirely sure what to check here? Should I regenerate the docs for the site and just ensure that they look ok? |
clang/docs/ClangFormatStyleOptions.rst | ||
---|---|---|
216–221 | It seems that you modified Format.h but didn't re-run clang/docs/tools/dump_format_style.py (example for Left is actually right-aligned). |
addressed the remaining review comment
clang/docs/ClangFormatStyleOptions.rst | ||
---|---|---|
216–221 | Got it. Sorry for the confusion. |
LGTM. That's a great piece work @feg208. Thank you!
I've added many nit comments, but I didn't do it for all code comments.
Please check that all comments are full phrases (with full stops :) ) before landing.
Some comments are in .rst but you know that you need to update Format.h and then regenerate .rst :).
Also, don't hesitate to mark comments as done.
Do you need somebody to land it on your behalf?
clang/docs/ClangFormatStyleOptions.rst | ||
---|---|---|
224–229 | This repetition of demo struct seems not very useful. | |
258 | Nit. This can be done when landing. No need to update the patch just for this. | |
clang/include/clang/Format/Format.h | ||
93 | ||
clang/lib/Format/FormatToken.h | ||
434–442 | Nits. | |
clang/lib/Format/WhitespaceManager.cpp | ||
985 | ||
996 | ||
1004–1005 | ||
1053 | ||
1055 | ||
clang/lib/Format/WhitespaceManager.h | ||
184–189 | Nit. | |
264 | Nit. |
Awww thanks. I learned a lot from all the comments honestly. I appreciate the patience.
I've added many nit comments, but I didn't do it for all code comments.
Please check that all comments are full phrases (with full stops :) ) before landing.
Some comments are in .rst but you know that you need to update Format.h and then regenerate .rst :).
Also, don't hesitate to mark comments as done.
I'll roll these up.
Do you need somebody to land it on your behalf?
I do. I don't have commit rights
Please post the name and email for the commit. And if no one else commits, I will do it on the weekend.
clang/test/Format/struct-array-initializer.cpp | ||
---|---|---|
5 ↗ | (On Diff #351734) | This test is very flaky https://lab.llvm.org/buildbot/#/builders/109 ******************** TEST 'Clang :: Format/struct-array-initializer.cpp' FAILED ******************** Script: -- : 'RUN: at line 1'; grep -Ev "// *[A-Z-]+:" /b/1/clang-x86_64-debian-fast/llvm.src/clang/test/Format/struct-array-initializer.cpp | /b/1/clang-x86_64-debian-fast/llvm.obj/bin/clang-format -style="{BasedOnStyle: LLVM, AlignArrayOfStructures: Right}" /b/1/clang-x86_64-debian-fast/llvm.src/clang/test/Format/struct-array-initializer.cpp | /b/1/clang-x86_64-debian-fast/llvm.obj/bin/FileCheck -strict-whitespace -check-prefix=CHECK1 /b/1/clang-x86_64-debian-fast/llvm.src/clang/test/Format/struct-array-initializer.cpp : 'RUN: at line 4'; grep -Ev "// *[A-Z-]+:" /b/1/clang-x86_64-debian-fast/llvm.src/clang/test/Format/struct-array-initializer.cpp | /b/1/clang-x86_64-debian-fast/llvm.obj/bin/clang-format -style="{BasedOnStyle: LLVM, AlignArrayOfStructures: Left}" /b/1/clang-x86_64-debian-fast/llvm.src/clang/test/Format/struct-array-initializer.cpp | /b/1/clang-x86_64-debian-fast/llvm.obj/bin/FileCheck -strict-whitespace -check-prefix=CHECK2 /b/1/clang-x86_64-debian-fast/llvm.src/clang/test/Format/struct-array-initializer.cpp -- Exit Code: 141 Command Output (stderr): -- + : 'RUN: at line 1' + /b/1/clang-x86_64-debian-fast/llvm.obj/bin/clang-format '-style={BasedOnStyle: LLVM, AlignArrayOfStructures: Right}' /b/1/clang-x86_64-debian-fast/llvm.src/clang/test/Format/struct-array-initializer.cpp + grep -Ev '// *[A-Z-]+:' /b/1/clang-x86_64-debian-fast/llvm.src/clang/test/Format/struct-array-initializer.cpp + /b/1/clang-x86_64-debian-fast/llvm.obj/bin/FileCheck -strict-whitespace -check-prefix=CHECK1 /b/1/clang-x86_64-debian-fast/llvm.src/clang/test/Format/struct-array-initializer.cpp -- ******************** |
clang/test/Format/struct-array-initializer.cpp | ||
---|---|---|
5 ↗ | (On Diff #351734) | Yeah. I added you to a review to alter it to something I think will be less of an issue. Thanks by the by for getting the unused NetWidth variable |
The test is still intermittently failing on bots.
Is there some non-determinism in the change?
It may be a good idea to revert this.
Do we actually need this lit test? Mostly we only add lit tests when we add a new command line argument, I feel your are just testing what we also test in the unittests.
I think, at this point, the sensible thing to do is to pull the test. As @MyDeveloperDay points out it isn't really adding any value above and beyond the unit tests. If it were non-determinism in the change itself the unit tests would catch it. They test the same formatting and test for non-determinism explicitly. Yet they've never failed. And there are quite a few of them.
Another +1 for it failing intermittently on the bot I watch over, llvm-clang-x86_64-sie-ubuntu-fast. Anything that can be done to stabilize the test or to remove it would be appreciated!
I actually have two reviews up to remove the test https://reviews.llvm.org/D104295 or change it https://reviews.llvm.org/D104242 that have both been accepted but I don't have commit rights against llvm so I need to get someone to push them in. At this point removing the test is the least risky option. I am sorry for the delay and bother. Hopefully I can either get someone to put it in on my behalf or I'll get commit rights. I have followed the dev docs and emailed Chris.
There are a number of bugs logged against this feature, are you still around to look into them?
https://bugs.llvm.org/show_bug.cgi?id=51926
https://bugs.llvm.org/show_bug.cgi?id=51935
Don't forget to re-generate .rst.