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

CondSelectGadget for UInts #20

Merged
merged 4 commits into from Nov 21, 2020
Merged

CondSelectGadget for UInts #20

merged 4 commits into from Nov 21, 2020

Conversation

nirvantyagi
Copy link
Contributor

This pull request is copied over from arkworks-rs/snark#312

It implements CondSelectGadget for UInt constraint variables. UInt8 is implemented directly, while a macro is included to generate the implementation for UInt16, UInt32, UInt64.

A couple of things for review:

  • Is the tracing macro being used correctly? Should any of the inputs be skipped?
  • Is the behavior for selecting the value when the inputs have no values correct?

src/bits/uint.rs Outdated Show resolved Hide resolved
src/bits/uint.rs Outdated Show resolved Hide resolved
src/bits/uint8.rs Outdated Show resolved Hide resolved
src/bits/uint8.rs Outdated Show resolved Hide resolved
src/bits/uint.rs Outdated Show resolved Hide resolved
src/bits/uint8.rs Outdated Show resolved Hide resolved
src/bits/uint8.rs Outdated Show resolved Hide resolved
@Pratyush
Copy link
Member

Amazing, thanks! Left some comments!

Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
@nirvantyagi
Copy link
Contributor Author

Suggestions all looked good to me. What does the skip parameter in the tracing macro do? How do you decide which arguments to pass in?

@Pratyush
Copy link
Member

By default tracing allows you to record values of variables, but only if they implement Debug. Because for our use case the debug value of a variable doesn't contain useful information anyway, we just skip it.

@Pratyush
Copy link
Member

Ahh I think the formatting got messed up, so if you could format then I can merge!

@nirvantyagi
Copy link
Contributor Author

Oops, looks like my cargo fmt call made some changes the CI didn't like. What should I be doing to format?

@Pratyush
Copy link
Member

Sorry just saw this; you want to run cargo +stable fmt.

@nirvantyagi
Copy link
Contributor Author

Cool, done! Thanks for reviewing!

By the way, I was on nightly because the crate wasn't building on stable -- there was an error with the semver-parser crate.

@Pratyush Pratyush merged commit 77dfd7d into arkworks-rs:master Nov 21, 2020
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.

None yet

2 participants