-
Notifications
You must be signed in to change notification settings - Fork 57
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
Adds Value.matches #204
Adds Value.matches #204
Conversation
Codecov Report
@@ Coverage Diff @@
## master #204 +/- ##
==========================================
- Coverage 82.75% 82.48% -0.28%
==========================================
Files 33 33
Lines 5340 5360 +20
Branches 1145 1151 +6
==========================================
+ Hits 4419 4421 +2
- Misses 795 813 +18
Partials 126 126
Continue to review full report at Codecov.
|
n = len(self) | ||
if isinstance(value, int): | ||
value = Const(value) | ||
if isinstance(value, Const): |
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.
I don't think this is appropriate: Case
does not recognize Const
. If it is extended (which I'm not sure it should be), then both match
and Case
should be extended at the same time.
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.
One reason I don't think it should recognize Const
is that it's not clear how the length of the constant should be accounted for. For example, in the condition right below, you are emitting a warning "comparison will never be true" that would be triggered on e.g. Const(1, 1).match(Const(1, 10))
, whereas the comparison in that case will always be true.
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.
Done.
If this looks good, should I modify so that we can accept more than one value?
|
Please do. I'll take one more look at the error messages to make sure they're consistent with the ones in |
Sorry about the additional commits -- apparently that's what syncing a fork onto upstream/master does. And I don't see code coverage kicking off, either, so it's possible this commit (pretty much like anything I do with git more complicated than "submit this set of files") is borked. |
I'll just merge it locally. Will take a look somewhere in a few hours. |
I think codecov updates the very first comment each time it finishes? |
I'm confused. Did you delete all tests? |
Implemented in master. |
Allows expressions like
s.matches(37)
ands.matches("1---000")
Resolved issue.
Tested:
This properly translates to il:
And to Verilog: