Implement bitwise operators #755
Conversation
That sounds like a reasonable solution. I think it would be good, having an |
|
Updated. I'm not sure why all the builds are all failing now though? I'm able to build it just fine on my workstation. |
|
Thank you. I think the failing builds are related to #752. It should be an easy fix, which I can look into tonight. |
|
I fixed the error, added documentation and updated the add-in. |
numbat/src/number.rs
Outdated
| type Output = Number; | ||
|
|
||
| fn shl(self, rhs: Self) -> Self::Output { | ||
| Number(((self.0 as i64) << rhs.0 as i64) as f64) |
There was a problem hiding this comment.
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 as casts...
For example:
>>> 1 << 63
1 << 63
= -9.22337e+18
>>> 1 << 64
thread 'main' panicked at numbat/src/number.rs:167:16:
attempt to shift left with overflow
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
There was a problem hiding this comment.
I'm also not sure about these as casts...
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:
- unbounded: Any overflowing bits are dropped. If the rhs is larger than the type being shifted then all the bits are dropped resulting in a value of 0.
- wrapping: Masks off the higher order rhs bits that would cause an overflow to occur.
- checked: Returns None if the size of the rhs is larger than the size of the type being shifted.
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.
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.
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.1 silently 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.
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.
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.
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.
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.1 silently truncates the .5 and .1 and gives the value of 7.
Yeah that would not be ideal.
We could probably add some error checking that only allows bitwise operations to be performed when both arguments are scalars.
This sounds like a good idea to me, but I think this decision should be up to @sharkdp.
I would definitely need some help with the ffi function implementation.
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.
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.
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.
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.
Performing a bitwise operation on a float with a fractional value is undefined behavior. If it looks like an integer, it is safe to type cast. If the number value has a fractional component, it is likely better to bail out with a runtime error rather than silently truncating the fractional part.
Overflows during bit shift now are handled using the unbounded_shl approach. Bit shifts with negative values on the rhs of the operation also now report a runtime error.
The code in this pull request adds implementations for the following bitwise operators:
Note that typically used '^' character for xor currently clashes with the power operator in numbat, so I have used the more cumbersome to type, but unique ⨁ character for performing the xor operation.