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

Adding signed cmov #6

Merged
merged 7 commits into from
Oct 4, 2021

Conversation

wjuan-mob
Copy link
Contributor

Motivation
Signed cmov would be useful for some algorithms e.g. Circuit Oram.

In this PR
Refactor unsigned cmov to generic to add signed cmov.
Added signed cmov for i32 and i64
Added test for signed cmov

pub fn cmov_u32(condition: bool, src: &u32, dest: &mut u32) {
fn cmov_numeric<T>(condition: bool, src: &T, dest: &mut T)
where
T: Copy,
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, this trait bound should be on Cmov, since Copy can be (and is) derived for compound types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this function.

@cbeck88
Copy link
Contributor

cbeck88 commented Oct 4, 2021

So, I can see why you think that making this generic is good, because it will reduce code duplication, which often makes things more maintainable.

But, in this case, I'm worried that it is making the code much less maintainable, because there is no type-safety in inline assembly, and using generic parameters makes it very hard for me to reason about what types might be there.

So I would much prefer that we undo the cmov_numeric thing, and do not have any generic functions that contain inline assembly, because I think it will be much more challenging to maintain cmov_numeric than what we had before.

@wjuan-mob wjuan-mob closed this Oct 4, 2021
@wjuan-mob wjuan-mob reopened this Oct 4, 2021
@wjuan-mob
Copy link
Contributor Author

So, I can see why you think that making this generic is good, because it will reduce code duplication, which often makes things more maintainable.

But, in this case, I'm worried that it is making the code much less maintainable, because there is no type-safety in inline assembly, and using generic parameters makes it very hard for me to reason about what types might be there.

So I would much prefer that we undo the cmov_numeric thing, and do not have any generic functions that contain inline assembly, because I think it will be much more challenging to maintain cmov_numeric than what we had before.

Removed the generic and using core::mem::transmute instead.

Copy link
Contributor

@cbeck88 cbeck88 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@wjuan-mob wjuan-mob merged commit 0a8d814 into mobilecoinfoundation:master Oct 4, 2021
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