Skip to content

Fix module-level alignment breaking after non-alignable constructs - #2500

Open
dannyoler wants to merge 1 commit into
chipsalliance:masterfrom
dannyoler:fix-module-item-alignment-grouping
Open

Fix module-level alignment breaking after non-alignable constructs#2500
dannyoler wants to merge 1 commit into
chipsalliance:masterfrom
dannyoler:fix-module-item-alignment-grouping

Conversation

@dannyoler

Copy link
Copy Markdown

Summary

  • AlignModuleItems was the only alignment function that did not split partitions by blank lines before classifying groups
  • When a non-alignable construct (e.g. module instantiation, macro call) appeared in the module body, GetConsecutiveModuleItemGroups returned kNoMatch, permanently breaking alignment for all subsequent assign statements and declarations in that scope
  • Added GetModuleItemGroupsBetweenBlankLines which first partitions by blank lines, then runs GetConsecutiveModuleItemGroups within each sub-range — matching the pattern used by every other alignment function

Before

    assign short_a            = 1;  // aligned
    assign really_long_name_b = 2;  // aligned
    inst #() u (.Z(z));             // breaks alignment permanently
    assign c = 3;                   // NOT aligned
    assign longer_d = 4;            // NOT aligned

After

    assign short_a            = 1;  // aligned (group 1)
    assign really_long_name_b = 2;  // aligned (group 1)
    inst #() u (.Z(z));             // ends group 1, does not affect group 2
    assign c        = 3;            // aligned (group 2)
    assign longer_d = 4;            // aligned (group 2)

Blank lines continue to separate alignment groups as expected. Also removes the resolved TODO(b/161814377) comment.

Test plan

  • Verified on a 7400-line production RTL file with module instantiations, DFF macros, and hundreds of assign statements
  • Small test case confirms: instantiations don't break alignment, blank lines do break alignment
  • Existing verible tests should pass (alignment functions for ports, structs, enums, etc. already use blank-line splitting)

Related issues: #2021, #2481

@linux-foundation-easycla

linux-foundation-easycla Bot commented Mar 27, 2026

Copy link
Copy Markdown

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: dannyoler / name: Danny Oler (13fba42)

AlignModuleItems was the only alignment function that did not split
partitions by blank lines before classifying alignment groups.  When
a non-alignable construct (e.g. a module instantiation) appeared in
the module body, GetConsecutiveModuleItemGroups returned kNoMatch,
which permanently broke the alignment group for all subsequent
assign statements and declarations in that scope.

This change adds GetModuleItemGroupsBetweenBlankLines, which first
partitions by blank lines (using GetSubpartitionsBetweenBlankLines),
then runs GetConsecutiveModuleItemGroups within each sub-range.
This matches the pattern used by every other alignment function
(AlignPortDeclarations, AlignStructUnionMembers, AlignCaseItems,
etc.).

Before this fix:
  assign short_a            = 1;  // aligned
  assign really_long_name_b = 2;  // aligned
  inst #() u (.Z(z));             // breaks alignment permanently
  assign c = 3;                   // NOT aligned
  assign longer_d = 4;            // NOT aligned

After this fix:
  assign short_a            = 1;  // aligned (group 1)
  assign really_long_name_b = 2;  // aligned (group 1)
  inst #() u (.Z(z));             // ends group 1, does not affect group 2
  assign c        = 3;            // aligned (group 2)
  assign longer_d = 4;            // aligned (group 2)

Blank lines continue to separate alignment groups as expected.

Also removes the resolved TODO(b/161814377) comment, as continuous
assignment alignment support already exists and now works correctly.
@dannyoler
dannyoler force-pushed the fix-module-item-alignment-grouping branch from 080095c to 13fba42 Compare March 27, 2026 21:21
@hzeller

hzeller commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Thanks for your contribution. Sorry for the delay, was swamped with other projects and missed the mail from github about the change.

Can you have a look at the tests and make them all pass ?
(you probably also have to rebase)

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.

2 participants