Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

formatter: Fix constraint block expansion #761

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Apr 15, 2021

Fix expansion of constraint declarations with flattened constraint blocks:

{ ([<auto>], policy: always-expand) @{}
  { ([<auto>], policy: fit-else-expand) @{25}, (origin: "constraint ... : 1000]};}")
    { ([constraint num_trans_c {], policy: fit-else-expand) }
    { (>>[num_trans inside { [ 800 : 1000 ] } ;], policy: always-expand, (origin: "num_trans i...0 : 1000]};")) }
    { ([}], policy: uninitialized) }
  }
}

by propagation of partition policy:

{ ([<auto>], policy: always-expand) @{}
  { ([<auto>], policy: always-expand) @{25}, (origin: "constraint ... : 1000]};}")
    { ([constraint num_trans_c {], policy: fit-else-expand) }
    { (>>[num_trans inside { [ 800 : 1000 ] } ;], policy: always-expand, (origin: "num_trans i...0 : 1000]};")) }
    { ([}], policy: uninitialized) }
  }
}

Constraint block is not expaned when constraint block item list contains
only one expression. This is caused by flattening partition with one
children and skipping force expansion on partition with one or zero
childrens (even with always-expand policy).

This patch fixes this by propagation of always-expand policy to its
parent, e.g. kConstraintDeclaration.

Signed-off-by: Lukasz Dalek <[email protected]>
@googlebot googlebot added the cla: yes All contributors in pull request have signed the CLA with Google. label Apr 15, 2021
@ghost ghost mentioned this pull request Apr 15, 2021
@ghost ghost requested a review from hzeller April 15, 2021 17:23
Copy link
Collaborator

@hzeller hzeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Ready to merge once CI is through.

@ghost
Copy link
Author

ghost commented Apr 15, 2021

Okay, great. But let's wait for a clarification in an issue thread.

nikhiljha pushed a commit to nikhiljha/verible that referenced this pull request Sep 27, 2022
* chore: update some logging

* docs: add section about known errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes All contributors in pull request have signed the CLA with Google.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants