[PATCH] D101868: [clang-format] Adds a formatter for aligning arrays of structs

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D101868#2777423 <https://reviews.llvm.org/D101868#2777423>, @feg208 wrote:

> In D101868#2776633 <https://reviews.llvm.org/D101868#2776633>, @HazardyKnusperkeks wrote:
>
>> In D101868#2775550 <https://reviews.llvm.org/D101868#2775550>, @feg208 wrote:
>>
>>> 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?
>
> 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.



================
Comment at: clang/lib/Format/TokenAnnotator.cpp:2650
+
+// NOLINTNEXTLINE(misc-no-recursion)
+FormatToken *TokenAnnotator::calculateInitializerColumnList(
----------------
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.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:2662
+  while (CurrentToken != nullptr && CurrentToken != Line.Last) {
+    if (CurrentToken->is(tok::l_brace)) {
+      ++Depth;
----------------
Although I'm not a big fan of it, I think the LLVM style is no braces for one line if and else.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:2681
+                                                CurrentToken->Next, Depth);
+          // NOLINTNEXTLINE(llvm-else-after-return)
+        } else if (Depth == 1) {
----------------
This one I'm against, just do what clang-tidy says remove the else. (Others may disagree.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101868