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

Impl Wrapping* traits from num-traits #425

Merged
merged 4 commits into from
Dec 15, 2023
Merged

Impl Wrapping* traits from num-traits #425

merged 4 commits into from
Dec 15, 2023

Conversation

tarcieri
Copy link
Member

@tarcieri tarcieri commented Dec 14, 2023

Until now we have used traits defined in this crate, primarily because many of these traits are predicates that return bool and in constant-time code we want those to return Choice/CtOption.

Wrapping* is an example of the sort of trait we'd want to incorporate unchanged, and there are potentially others, so this adds num-traits as a hard dependency (previously our only hard dependency was subtle). Note that num-traits itself has no transitive dependencies (or rather, they're optional but not enabled).

The Wrapping* traits have bounds on the corresponding op traits being impl'd with Self operands e.g. WrappingAdd: Add<Self, Output = Self> so this PR also adds impls of those traits.

We've previously avoided these as in std they panic on overflow/underflow in debug builds and silently wrap in release builds. This PR always panics.

This required some changes to the Mul impls which were conditional on ConcatMixed and implicitly widened. To accomodate impls which are always available and require no bounds (in order to allow us to impl WideningMul), and renames the following:

  • Uint::mul -> Uint::widening_mul
  • Uint::mul_wide -> Uint::widening_mul_split

TODO:

  • WrappingNeg
  • WrappingShl
  • WrappingShr

cc @xuganyu96

@tarcieri tarcieri requested a review from fjarri December 14, 2023 17:10
@tarcieri
Copy link
Member Author

Until now we've avoided these sort of panicking impls, requiring the selection of either Wrapping or Checked semantics in order to ensure that overridden operators don't come with hidden panics.

Other PRs have added panics that brought the behavior more in-line with core/std semantics (#395) so I thought I'd float this approach.

I'm not entirely sure I like it, to be honest, but it can be annoying not to have a convenient way to perform an operation you're sure won't overflow on e.g. a BoxedUint without first having to place it in a Wrapping wrapper.

@tarcieri
Copy link
Member Author

The main argument in favor of this PR is probably UX/approachability: we've had several complaints this library is hard-to-use, and the absence of the core::ops traits on Uint<_> are a big part of that, IMO.

Adding them should make this library easier to learn because it will align the library with the core/std mental model of how things should operate, while also ensuring overflows don't silently happen in release builds.

Until now we have used traits defined in this crate, primarily because
many of these traits are predicates that return `bool` and in
constant-time code we want those to return `Choice`/`CtOption`.

`Wrapping*` is an example of the sort of trait we'd want to incorporate
unchanged, and there are potentially others, so this adds `num-traits`
as a hard dependency (previously our only hard dependency was `subtle`).
Note that `num-traits` itself has no transitive dependencies (or rather,
they're optional but not enabled).

The `Wrapping*` traits have bounds on the corresponding op traits being
impl'd with `Self` operands e.g. `WrappingAdd: Add<Self, Output = Self>`
so this PR also adds impls of those traits.

We've previously avoided these as in `std` they panic on
overflow/underflow in debug builds and silently wrap in release builds.
This PR always panics.

This required some changes to the `Mul` impls which were conditional on
`ConcatMixed` and implicitly widened. To accomodate impls which are
always available and require no bounds (in order to allow us to impl
`WideningMul`), and renames the following:

- `Uint::mul` -> `Uint::widening_mul`
- `Uint::mul_wide` -> `Uint::widening_mul_split`
@tarcieri tarcieri merged commit 3819a4d into master Dec 15, 2023
@tarcieri tarcieri deleted the wrapping-traits branch December 15, 2023 02:35
@tarcieri tarcieri mentioned this pull request Jan 22, 2025
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.

1 participant