mojo
mojo copied to clipboard
modularml
[mojo-stdlib] implement shifts in object
@JoeLoser Hi, I have a questions in regards to all the bitwise operation for object. Python's behavior for shift and bitwise and/or/xor is different in that python will always cast the type of shift to an int where as the other bitwise operation will depend on the input types.
In this pr I kind of just do a type check and then return a new object of type int for all the shift operator. Mean while I also found a lot of other bitwise operator such as and/or/xor that only support bool type https://github.com/modularml/mojo/blob/abf2d97e88a2cc97e3029a578d2824895a88d0c1/stdlib/src/builtin/object.mojo#L1315 https://github.com/modularml/mojo/blob/abf2d97e88a2cc97e3029a578d2824895a88d0c1/stdlib/src/builtin/object.mojo#L1330
So I create another function name _arithmetic_bitwise_op() that handles these kind of operator for both int and bool. I did not use this function for the shift operation as it is not necessary, but it would be very helpful for completing the rest of the bitwise operator.
Please let me know if it is OK to include that into this pr. The rest of the bitwise operator can be completed once this pr is merged. Thanks!
Looks good, thank you! You'll need to rebase to resolve the test file conflict before we can merge.
Hi @LJ-9801, thanks for this contribution! Joe and I talked about your question above re. bitwise operations. What you said about the other bitwise operators makes sense, but since it's separate from the problem of supporting shift operators, it would make review and discussion easier if we make those changes in a separate PR. This PR looks good as is though :)
Since _arithmetic_bitwise_op is not used in this PR, I suggest we leave it out of this PR, and include it in a follow-up PR making the changes to the other bitwise operators you mentioned.
Thanks again for this contribution, very usefull! :fire:
@JoeLoser @ConnorGray the function has been removed from this PR. I will open a issue for this as well as all the bitwise operation!
@JoeLoser @ConnorGray the function has been removed from this PR. I will open a issue for this as well as all the bitwise operation!
Sounds good, feel free to do that. I've just merged this PR. Thanks again for the contribution! 🎉