feg208 added a comment. 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101868/new/ https://reviews.llvm.org/D101868 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
