-
Notifications
You must be signed in to change notification settings - Fork 64
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
Handling of unknown width for multiports #2057
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! We should probably add the failing code from #1833 as a test case.
I don't know how to add a test for this. The NPE occurred during diagram synthesis. During compilation, you only get a a warning. Note that fundamentally, because in Cpp the widths of banks and multiports may not be knowable before executing the code, you have no assurance that the diagram is correct. My fix errs on the side of making more connections rather than fewer. There are probably examples where you will get false reports of causality loops. |
Thanks for fixing this Edward! I think it is OK to assume more connections (and be conservative in cycle detection). Situations where this can go wrong are rather obscure. Since we introduced the lfd tool, we also test diagram generation in CI. However, it is not fully functional at the moment, as I didn't figure out how to draw the diagrams fully expanded yet. So I am not sure if the bug would be triggered. In any case, I think it would be good to add a testcase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving this because having the fix is more important than not having the test.
This fixes #1833.