zachjs / sv2v

SystemVerilog to Verilog conversion
BSD 3-Clause "New" or "Revised" License
497 stars 50 forks source link

Print blocked statement to avoid dangling else even when it would be parsed correctly #244

Closed dwRchyngqxs closed 1 year ago

dwRchyngqxs commented 1 year ago

The dangling else is a grammar ambiguity, as such sv2v should always avoid generating it. It is already done for statements, but only in the case where the else is attached to the outside if, not the inside it (common way to parse the dangling else).

Summary of changes: Removed begin/end block when the inner is a DoWhile because that would already enclose the else. Dangling else check passes through attributes (bug-fix). Dangling else check passes through most control flow structures instead of always enclosing inside begin/end. Added test case.

zachjs commented 1 year ago

Thank you very much for finding the issue with statement attributes and sharing this PR! The changes WRT the control flow constructs seem reasonable too. I simplified the test you wrote a bit, but it looks great overall.

Can you elaborate on the ambiguity you perceive? All three of IEEE 1364-2001, 1364-2005, and 1800-2017 all state:

Because the else part of an if-else is optional, there can be confusion when an else is omitted from a nested if sequence. This is resolved by always associating the else with the closest previous if that lacks an else.

I'm not opposed to making this change, but I'm wondering if you have a particular downstream tool or use case in mind given that it would indeed be parsed correctly either way.

dwRchyngqxs commented 1 year ago

The grammar is ambiguous, the standard states how to solve this ambiguity. Even though the standard states how to solve the ambiguity I wouldn't rely on downstream tools parsing a dangling else correctly. I don't have any tool in mind that doesn't parse it correctly, and even if I did, a bug in another tool wouldn't justify this change. In my mind there are 2 justifications: