zachjs / sv2v

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

SV2V automatically removes parentheses in operator precedence #268

Closed anhdv2000 closed 5 months ago

anhdv2000 commented 6 months ago

I have the expression in1 | (| in2) , and after using the sv2v tool, it removed the parentheses and became in1 | |in2. Personally, I think to make the expression clearer, the tool should not remove parentheses. Could you please consider reviewing and improving your tool?

zachjs commented 6 months ago

Personally, I think to make the expression clearer, the tool should not remove parentheses.

Can you help me understand your downstream use case where the existing output of sv2v doesn't suit your needs? Are you feeding it into a downstream tool? Editing the generated output manually? Something else?

anhdv2000 commented 6 months ago

@zachjs I used Spyglass to check the .v code generated by SV2V and encountered the warning WRN_61 (547): Parentheses should be used with the reduction AND operator following the bitwise AND operator [a&(&b)]. I think you shouldn't remove the parentheses in this case; it both conforms to the standard and provides clarity for designers

zachjs commented 6 months ago

sv2v's frontend, where the text is turned into an AST, doesn't distinguish x = y; from x = (y); from x = ((((y))));. In this sense, certain parentheses are always "removed". I do not intend to adjust sv2v's frontend to preserve such parentheses.

Instead, we could change the behavior of sv2v's backend, where the converted AST is turned back into text, to adhere to some particular stylistic preferences.

Can you confirm that you are requesting only these two specific stylistic changes, where x and y are arbitrary expressions?

I personally disagree with Spyglass here, but I'm willing to make changes to sv2v's backend where the complexity is low and the scope is narrow. Though I might implement the above request, I do not necessarily intend to make sv2v's output Spyglass compliant.

I'm also not really sure that the existing output is unclear. There are at least a few other cases in SystemVerilog where spacing distinguishes between two valid parses.

anhdv2000 commented 5 months ago

@zachjs I completely agree with you on this. It's just a warning in Spyglass, not a big concern. However, I think, for code readability, it would be beneficial to include parentheses for frequently used operations to avoid misunderstandings (even though it already has spaces for distinction). It also reduces warnings for downstream tool. I think changing just these two specific stylistic elements is sufficient

I believe its scope is quite narrow and only slightly adjusts the code representation. So, it would be great if you're willing to make these changes. Thank you !!!

zachjs commented 5 months ago

I just pushed 9825bb9bcbabde39e3cb5e6f3ac041fa2a081266 to make this change. Please let me know if it works for you.

zachjs commented 5 months ago

I'm closing this because I think the original request is now satisfied. Please let me know if you have further issues!