Fix formatter non-convergence for continuation EOL comments - #2543
Fix formatter non-convergence for continuation EOL comments#2543kbrunham-intel wants to merge 5 commits into
Conversation
Count only the width after the last newline in preserved leading spaces when placing continuation comments after wrapped lines, so re-formatting keeps a stable column (issue chipsalliance#2542).
|
Sorry, due to other changes, the formatter_test.cc now needs merging. Can you rebase ? |
| line.Tokens().end() - 1)) { | ||
| AdjustColumnUsingTokenSpacing(ftoken, &column); | ||
| column += ftoken.token->text().length(); | ||
| for (size_t i = 1; i < tokens.size(); ++i) { |
There was a problem hiding this comment.
since we're using tokens here first, I'd probably move the const auto &tokens = line.Tokens(); right in front of the for loop.
| column += ftoken.token->text().length(); | ||
| for (size_t i = 1; i < tokens.size(); ++i) { | ||
| const auto &ftoken = tokens[i]; | ||
| switch (ftoken.before.action) { |
There was a problem hiding this comment.
The previous implementation, that put the logic into its own function (AdjustColumnsUsingTokenSpacing()), was nicely separating that dense logic into the function call. Do you think we can do something here as well again ?
There was a problem hiding this comment.
I've created a new helper function here to help with readability.
Resolve formatter_test.cc conflict by keeping ContinuationCommentAfterWrappedAssignConverges alongside master's stacked regression tests.
Move tokens lookup before the loop and factor spacing logic back into AdjustColumnUsingTokenSpacing with the chipsalliance#2542 newline-aware preserve fix (PR review feedback).
iterator-range and absl core_headers are unused after the chipsalliance#2542 refactor dropped make_range and ABSL_FALLTHROUGH_INTENDED.
|
Hi @hzeller. I've updated the PR and provided a change to incorporate your feedback. Please approve and merge the change if it is ready, or let me know what other feedback I should look to incorporate. |
2 participants
Summary
assignis followed by column-aligned continuation EOL comments (format(format(x)) != format(x)).CalculateEolCommentColumnnow treats newlines in preserved leading whitespace like emit does (reset column after the last\n) instead of counting them as width.ContinuationCommentAfterWrappedAssignConverges.Fixes #2542
Test plan
bazel test -c opt //verible/verilog/formatting:formatter_test