Skip to content
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

Int - Bit ops #697

Merged
merged 13 commits into from
Nov 12, 2024
Merged

Int - Bit ops #697

merged 13 commits into from
Nov 12, 2024

Conversation

erik-3milabs
Copy link

@erik-3milabs erik-3milabs commented Oct 22, 2024

Bit operations on Int, in preparation for implementing Integer trait.

@erik-3milabs erik-3milabs changed the title Bit ops Int - Bit ops Oct 22, 2024
@erik-3milabs erik-3milabs changed the title Int - Bit ops Int - Bit ops Oct 22, 2024
This was referenced Oct 22, 2024
@erik-3milabs erik-3milabs marked this pull request as ready for review November 4, 2024 11:19
Comment on lines +458 to +461
pub const fn expect(self, msg: &str) -> Int<LIMBS> {
assert!(self.is_some.is_true_vartime(), "{}", msg);
self.value
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copy-pasted this from ConstCtOption<Uint>.

In doing so, I did notice that, although it uses is_true_vartime, ConstCtOption<Uint>::expect was not annotated as being vartime. Moreover, it is used for Uint::shr and Uint::shl, among others, which are not vartime either. Is this OK because unwraps are vartime anyway, or do we need to do something about this @tarcieri ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're not really "vartime" in that if the condition is violated, it will crash the entire program, as opposed to completing successfully in a variable number of CPU cycles

@erik-3milabs
Copy link
Author

@tarcieri this is the next PR ready for review. I am looking forward to your input!

Copy link

@lleoha lleoha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,
Although I am wondering if we also need unsigned right shifts for Ints.
Sorry for taking this so long, I was out of reach :).

@erik-3milabs
Copy link
Author

@tarcieri ping 😄

@erik-3milabs
Copy link
Author

@lleoha IMHO, I see no added value in supporting logical (=unsigned) right shift on a signed integer: it could be confusing for users, and the difference is quite subtle.

Moreover, when logically shifting-right a negative value, you get some bizarre behaviour. To illustrate with an i8:
-122i8.logical_shr(1) = 60. It is not even abs_div_by_2 (which should be 61). I see no reason to support it, but please let me know if you have a use case in which it does make sense!

Moreover, if for some reason you really need the logical shift right, the solution x.as_uint().shr(1).as_int() is still on the table, and it immediately illustrates to the reader something weird is up and that they should pay attention.

@lleoha
Copy link

lleoha commented Nov 11, 2024

@lleoha IMHO, I see no added value in supporting logical (=unsigned) right shift on a signed integer

I'm sold :).
Let's not add it then.

@tarcieri tarcieri merged commit 986faf2 into RustCrypto:signed-int Nov 12, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants