-
-
Notifications
You must be signed in to change notification settings - Fork 89
Implement bitwise operators #755
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
Open
rehelmin
wants to merge
11
commits into
sharkdp:master
Choose a base branch
from
rehelmin:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
04d62f3
Implement bitwise operators.
09aec75
Cargo fmt formatting updates.
038c142
Add 'xor' as alternative to ⨁ for performing xor operation.
81786f2
Merge branch sharkdp/master into rehelmin/master
Goju-Ryu 61ea3d2
Fix compile error
Goju-Ryu bfaae31
Add 'xor' keyword
Goju-Ryu b46f890
Add variant of 'xor' unicode character
Goju-Ryu 9c18567
Add bitwise operators to documentation
Goju-Ryu 95d8171
Check that operands of bitwise operations are integer quantities.
4d1ba9d
Implement additional error checking.
a503b2b
Add skeleton ffi function implementation
Goju-Ryu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| ~2.2 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| # bitwise and truth table | ||
| assert_eq(1 & 1, 1) | ||
| assert_eq(1 & 0, 0) | ||
| assert_eq(0 & 1, 0) | ||
| assert_eq(0 & 0, 0) | ||
|
|
||
| # bitwise or truth table | ||
| assert_eq(1 | 1, 1) | ||
| assert_eq(1 | 0, 1) | ||
| assert_eq(0 | 1, 1) | ||
| assert_eq(0 | 0, 0) | ||
|
|
||
| # bitwise xor truth table | ||
| assert_eq(0 ⨁ 0, 0) | ||
| assert_eq(1 ⨁ 0, 1) | ||
| assert_eq(0 ⨁ 1, 1) | ||
| assert_eq(1 ⨁ 1, 0) | ||
|
|
||
| assert_eq(0 xor 0, 0 ⨁ 0) | ||
| assert_eq(1 xor 0, 1 ⨁ 0) | ||
| assert_eq(0 xor 1, 0 ⨁ 1) | ||
| assert_eq(1 xor 1, 1 ⨁ 1) | ||
|
|
||
| # bitshift left checks | ||
| assert_eq(0xFF << 2, 0x3FC) | ||
| assert_eq(0xFF << 0, 0xFF) | ||
|
|
||
| # bitshift right checks | ||
| assert_eq(0xC7A5 >> 8, 0xC7) | ||
|
|
||
| # bit flipping operation | ||
| assert_eq(0xC700 ⨁ (0xC0 << 8), 0x700) | ||
| assert_eq(0xC700 xor (0xC0 << 8), 0x700) | ||
|
|
||
| # bit clearing operation | ||
| assert_eq(0xC700 & ~(1 << 15), 0x4700) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Thank you for working on this!
I believe we need to write a few more test cases for extreme inputs, and we need to improve error handling here. These bitwise operations can overflow, and we don't want Numbat to crash in those cases (but rather emit some error, similar to the division-by-zero error). I'm also not sure about these
ascasts...For example:
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.
What aspect of them are you unsure about? It was the only way I could figure out how to work around the fact that the underlying primitive type for Number is f64. None of the bitwise operations make sense in the context of an f64 type.
Good point on the overflowing. Looks like there are a bunch of different options that have different behavior defined for overflows that could occur during the shifting operations:
My preference is the unbounded implementation, but I could be convinced that it would be better to throw an error rather than to silently fail in a defined way.
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 think it might be worth having more than one implementation, but I agree with preferring the unbounded version for the operator. The other versions could potentially be added as ffi functions to give the option if needed.
Disclaimer: I don't use bit shifting that much, so my preference for unbounded shifts is mostly just that they make more intuitive sense to me.
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 side effect of the type casts is that it silently truncates the decimal part of any floats that someone tries to perform a bitwise oepration on. For example
5.5 | 2.1silently truncates the .5 and .1 and gives the value of 7. I can see this being somewhat undesired. We could probably add some error checking that only allows bitwise operations to be performed when both arguments are scalars. I've already added this error checking for performing the bitwise not. I don't think it would be a big step to add it to the rest of the operations.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 would definitely need some help with the ffi function implementation. These updates are my first forays into the world of rust, so I've mostly been pattern matching off of existing code to implement them.
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.
Also, another point on the type casts is that it doesn't make sense to bit shift with a negative integer on the right-hand side of the operation. Rust throws an error in this scenario. I can fix that tonight.
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.
Yeah that would not be ideal.
This sounds like a good idea to me, but I think this decision should be up to @sharkdp.
No problem, I think I can help with that. I’m not sure when I can, but I'll drop some updates with the ffi connections and a todo for where the function implementation should go if you want it.
I’m learning rust in much the same way. Thank you for taking on the challenge to add this feature 😄
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've added a very simple ffi outline that should work for the left and right shifts. I've left comments with what still needs to be done before it's done.
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.
Yes, I think we should do that. Only scalars should be allowed to be
xored. At the moment, something like1 m xor 2 mleads to a panic.