-
Notifications
You must be signed in to change notification settings - Fork 68
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
[Issue-133] simcompare calculate signaltowidth instead of manual input #211
[Issue-133] simcompare calculate signaltowidth instead of manual input #211
Conversation
…ad-of-manual-input
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.
Looks great! Just a few changes
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.
Just the one comment that I reopened, other than that looks good!
Good catch; I learned that github reviews can show you old versions of code, and I think I reviewed an old version. I agree that null is better than '' for "no parameter passed". This probably means that '' should be checked for and an error reported. |
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.
Looks good!
I think since this isn't part of the public API, let's count this as not backwards-compatibility-breaking, especially since migrating for those who are using it is just a matter of deleting some information. Can you get this branch back up through merge conflicts and then we can merge it in? |
@mkorbel1 I resolved the merge conflict, do let me know if need anything else. |
Looks like you pulled in all the website changes as well into this branch? Then we should either revert that so it's just simcompare in here or else merge the website first |
Aww, I will remove the website. Looks like there were some mistakes there during my commit. |
be69868
to
0d756e3
Compare
@mkorbel1 I think it should be good to go now. I reset to previous unpolluted commit and remerge. |
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.
looks great!
Description & Motivation
It's annoying and confusing to need to set the signalToWidth map for SimCompare after already setting port widths properly.
Related Issue(s)
Fix #133
Testing
No new unit test been created.
Backwards-compatibility
Yes.
signalToWidth
parameter is no longer supported whileModule
must be provided to thesimcompare.iverilogVector()
.Documentation
Nope.