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