-
Notifications
You must be signed in to change notification settings - Fork 173
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
Add rotate by constant #352
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.
Please add some tests. In this case, some assertRepr
tests in test_hdl_ast.py
, and a functional test in test_sim.py
. In the latter, if you write the literals in binary, it'll be easy to see if you got the signs right.
Codecov Report
@@ Coverage Diff @@
## master #352 +/- ##
==========================================
+ Coverage 82.69% 82.72% +0.02%
==========================================
Files 35 35
Lines 5919 5940 +21
Branches 1201 1206 +5
==========================================
+ Hits 4895 4914 +19
- Misses 863 864 +1
- Partials 161 162 +1
Continue to review full report at Codecov.
|
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.
Please add tests for rotates by non-1, and in particular for rotates by values larger than the value size, since currently that would be an error.
Also, if you think negative rotates should be supported, tests for those, too. If you think they should not be, then add raise SyntaxError
if they are encountered. (Personally I am fine with either option.)
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.
Should be good after one more round of changes.
I think I got the signs right, anyway.
Closes #339.