Skip to content

Fix formatter non-convergence with EOL comment before else if - #2541

Open
kbrunham-intel wants to merge 1 commit into
chipsalliance:masterfrom
kbrunham-intel:fix/2540
Open

Fix formatter non-convergence with EOL comment before else if#2541
kbrunham-intel wants to merge 1 commit into
chipsalliance:masterfrom
kbrunham-intel:fix/2540

Conversation

@kbrunham-intel

@kbrunham-intel kbrunham-intel commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #2540: formatter non-convergence when an EOL comment sits between end and a following else if.

When end carried a trailing EOL comment, PushEndIntoElsePartition merged that end+comment into the else-if header partition. fit-else-expand then treated the header as wider than the eventual printed line, so the first format kept the else-if assignment on one line and the second wrap split it — format(format(x)) != format(x).

Change: skip merging end into the else partition when the end leaf contains an EOL comment (else must start on the next line anyway when preceded by a comment).

Test plan

  • Added FormatterEndToEndTest.EndElseIfWithEOLCommentConverges covering comment-on-own-line and comment-already-on-end
  • bazel test //verible/verilog/formatting:formatter_test
  • CI green on this PR

@linux-foundation-easycla

linux-foundation-easycla Bot commented Jul 17, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: kbrunham-intel / name: Kalen Brunham (af81883)

Skip merging end into the else partition when end has a trailing EOL
comment so fit-else-expand does not oscillate on re-format (issue chipsalliance#2540).
@kbrunham-intel kbrunham-intel changed the title Fix for issue 2540 Fix formatter non-convergence with EOL comment before else if Jul 17, 2026
@hzeller

hzeller commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Thanks for your contribution - Sorry for the delay in the review, I was a bit busy the last couple of weeks.

Looks good. Can you rebase, as there were other changes in the formatter_test that now require a merge ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Re-formatted text does not match formatted text; formatting failed to converge! Please file a bug.

2 participants