Page MenuHomePhabricator

[clang-format] Adds a formatter for aligning arrays of structs
ClosedPublic

Authored by feg208 on May 4 2021, 4:08 PM.

Details

Summary

This adds a new formatter to arrange array of struct initializers into neat columns

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
feg208 updated this revision to Diff 349434.Wed, Jun 2, 5:41 PM

Adds a simple lit test for some sanity checks

clang/lib/Format/TokenAnnotator.cpp
737–740

I actually meant so. Because now this is even safe if the iterators are not random access anymore in the future.

clang/lib/Format/WhitespaceManager.cpp
1145–1146

I don't know how to do this in LLVM style, but maybe so?

feg208 updated this revision to Diff 349589.Thu, Jun 3, 9:38 AM
feg208 marked 2 inline comments as done.

Grabs up some review comments and adds lit test

feg208 added a comment.Thu, Jun 3, 9:39 AM

Got both of these

clang/lib/Format/TokenAnnotator.cpp
737–740

Oh I see. I sort of wondered about that but it didn't seem like a hill to die on.

clang/lib/Format/WhitespaceManager.cpp
1145–1146

So long as clang-format doesn't complain I am fine either way

feg208 updated this revision to Diff 349728.Thu, Jun 3, 4:31 PM

Rebased against main

feg208 updated this revision to Diff 349730.Thu, Jun 3, 4:35 PM

Had some invalid code in the lit test

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
983

This is unneccessary. (But doesn't need a new revision until approved.)

1082

Same

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.

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

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!

Better still how about having

ArraryMemberAligmentEnum AlignArrayOfStructures

of "None, Left and Right"

I find boolean options don't stay boolean for long!

A strong +1.

feg208 added a comment.Tue, Jun 8, 6:35 AM

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

feg208 updated this revision to Diff 350746.Tue, Jun 8, 5:21 PM

This alters the array alignment to allow for left alignment

feg208 updated this revision to Diff 350774.Tue, Jun 8, 8:32 PM

Fixes from clang-tidy checks

curdeius added inline comments.Wed, Jun 9, 12:43 AM
clang/docs/ClangFormatStyleOptions.rst
216–221

Don't forget to re-generate .rst.

MyDeveloperDay added inline comments.Wed, Jun 9, 12:54 AM
clang/lib/Format/WhitespaceManager.h
234

Can ensure the comment starts with a capital and finishes with punctuation (I personally don't mind but I believe that is the convention)

275
292
311
314
feg208 updated this revision to Diff 350910.Wed, Jun 9, 8:44 AM
feg208 marked 4 inline comments as done.

Gets some of the formatting issues. Still chasing a test issue.

feg208 added a comment.Wed, Jun 9, 8:44 AM

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?

feg208 updated this revision to Diff 350920.Wed, Jun 9, 9:17 AM

Fixes the busted test

feg208 updated this revision to Diff 350924.Wed, Jun 9, 9:25 AM

Ick. Missed a semi colon in the test

curdeius added inline comments.Wed, Jun 9, 11:25 AM
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).

feg208 updated this revision to Diff 350974.Wed, Jun 9, 12:28 PM

Regenerated the mangled ClangFormatStyle.rst file

feg208 marked an inline comment as done.Wed, Jun 9, 12:29 PM

addressed the remaining review comment

clang/docs/ClangFormatStyleOptions.rst
216–221

Got it. Sorry for the confusion.

curdeius accepted this revision.Wed, Jun 9, 12:46 PM

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
213–218

This repetition of demo struct seems not very useful.

247

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
994
1005
1013–1014
1062
1064
clang/lib/Format/WhitespaceManager.h
185–190

Nit.

266

Nit.

This revision is now accepted and ready to land.Wed, Jun 9, 12:46 PM
feg208 marked an inline comment as done.Wed, Jun 9, 12:52 PM

LGTM. That's a great piece work @feg208. Thank you!

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

feg208 updated this revision to Diff 350984.Wed, Jun 9, 1:05 PM
feg208 marked 11 inline comments as done.

Rolls up the remaining review comments

feg208 marked 3 inline comments as done.Wed, Jun 9, 1:09 PM

All the review comments are addressed

If I can get someone to submit this on my behalf I think we can call it a day.

If I can get someone to submit this on my behalf I think we can call it a day.

Please post the name and email for the commit. And if no one else commits, I will do it on the weekend.

If I can get someone to submit this on my behalf I think we can call it a day.

Please post the name and email for the commit. And if no one else commits, I will do it on the weekend.

Fred Grim fgrim@apple.com

Thanks much

This revision was landed with ongoing or failed builds.Sun, Jun 13, 12:14 PM
This revision was automatically updated to reflect the committed changes.
vitalybuka added inline comments.
clang/test/Format/struct-array-initializer.cpp
5

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
--
********************
feg208 added inline comments.Mon, Jun 14, 8:34 PM
clang/test/Format/struct-array-initializer.cpp
5

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.

dyung added a subscriber: dyung.Tue, Jun 15, 12:31 PM

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!

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.