Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Remove Copy from Ensure* traits #13043

Merged
paritytech-processbot merged 8 commits into from
Jan 4, 2023
Merged

Remove Copy from Ensure* traits #13043

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8ffc145
Remove Copy from EnsureOp and EnsureOpAssign
ggwpez Jan 2, 2023
597e61d
Remove Copy from EnsureFrom and EnsureInto
ggwpez Jan 2, 2023
0bab4c5
Fix default impl
ggwpez Jan 2, 2023
95b1674
Reuse assignment code in Ensure trait
ggwpez Jan 2, 2023
8d3b649
Require Ensure for all BaseArithmetic types
ggwpez Jan 2, 2023
35c3cc8
Fix assign impls
ggwpez Jan 2, 2023
6c1e4d9
Add tests
ggwpez Jan 2, 2023
78e8611
Add success doc tests
ggwpez Jan 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 44 additions & 47 deletions primitives/arithmetic/src/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,11 +340,11 @@ impl<T: Sized> SaturatedConversion for T {}
/// The *EnsureOps* family functions follows the same behavior as *CheckedOps* but
/// returning an [`ArithmeticError`](crate::ArithmeticError) instead of `None`.
mod ensure {
use super::{CheckedAdd, CheckedDiv, CheckedMul, CheckedSub, Zero};
use super::{BaseArithmetic, CheckedAdd, CheckedDiv, CheckedMul, CheckedSub, Zero};
use crate::{ArithmeticError, FixedPointNumber, FixedPointOperand};

/// Performs addition that returns [`ArithmeticError`] instead of wrapping around on overflow.
pub trait EnsureAdd: CheckedAdd + PartialOrd + Zero + Copy {
pub trait EnsureAdd: EnsureAddAssign {
/// Adds two numbers, checking for overflow.
///
/// If it fails, [`ArithmeticError`] is returned.
Expand All @@ -368,13 +368,13 @@ mod ensure {
/// assert_eq!(underflow(), Err(ArithmeticError::Underflow));
/// ```
fn ensure_add(self, v: Self) -> Result<Self, ArithmeticError> {
self.checked_add(&v).ok_or_else(|| error::equivalent(v))
self.checked_add(&v).ok_or_else(|| error::equivalent(&v))
}
}

/// Performs subtraction that returns [`ArithmeticError`] instead of wrapping around on
/// underflow.
pub trait EnsureSub: CheckedSub + PartialOrd + Zero + Copy {
pub trait EnsureSub: CheckedSub + PartialOrd + Zero {
/// Subtracts two numbers, checking for overflow.
///
/// If it fails, [`ArithmeticError`] is returned.
Expand All @@ -398,13 +398,13 @@ mod ensure {
/// assert_eq!(overflow(), Err(ArithmeticError::Overflow));
/// ```
fn ensure_sub(self, v: Self) -> Result<Self, ArithmeticError> {
self.checked_sub(&v).ok_or_else(|| error::inverse(v))
self.checked_sub(&v).ok_or_else(|| error::inverse(&v))
}
}

/// Performs multiplication that returns [`ArithmeticError`] instead of wrapping around on
/// overflow.
pub trait EnsureMul: CheckedMul + PartialOrd + Zero + Copy {
pub trait EnsureMul: CheckedMul + PartialOrd + Zero {
/// Multiplies two numbers, checking for overflow.
///
/// If it fails, [`ArithmeticError`] is returned.
Expand All @@ -428,12 +428,12 @@ mod ensure {
/// assert_eq!(underflow(), Err(ArithmeticError::Underflow));
/// ```
fn ensure_mul(self, v: Self) -> Result<Self, ArithmeticError> {
self.checked_mul(&v).ok_or_else(|| error::multiplication(self, v))
self.checked_mul(&v).ok_or_else(|| error::multiplication(&self, &v))
}
}

/// Performs division that returns [`ArithmeticError`] instead of wrapping around on overflow.
pub trait EnsureDiv: CheckedDiv + PartialOrd + Zero + Copy {
pub trait EnsureDiv: CheckedDiv + PartialOrd + Zero {
/// Divides two numbers, checking for overflow.
///
/// If it fails, [`ArithmeticError`] is returned.
Expand All @@ -457,22 +457,22 @@ mod ensure {
/// assert_eq!(overflow(), Err(ArithmeticError::Overflow));
/// ```
fn ensure_div(self, v: Self) -> Result<Self, ArithmeticError> {
self.checked_div(&v).ok_or_else(|| error::division(self, v))
self.checked_div(&v).ok_or_else(|| error::division(&self, &v))
}
}

impl<T: CheckedAdd + PartialOrd + Zero + Copy> EnsureAdd for T {}
impl<T: CheckedSub + PartialOrd + Zero + Copy> EnsureSub for T {}
impl<T: CheckedMul + PartialOrd + Zero + Copy> EnsureMul for T {}
impl<T: CheckedDiv + PartialOrd + Zero + Copy> EnsureDiv for T {}
impl<T: CheckedAdd + PartialOrd + Zero> EnsureAdd for T {}
impl<T: CheckedSub + PartialOrd + Zero> EnsureSub for T {}
impl<T: CheckedMul + PartialOrd + Zero> EnsureMul for T {}
impl<T: CheckedDiv + PartialOrd + Zero> EnsureDiv for T {}

/// Meta trait that supports all immutable arithmetic `Ensure*` operations
pub trait EnsureOp: EnsureAdd + EnsureSub + EnsureMul + EnsureDiv {}
impl<T: EnsureAdd + EnsureSub + EnsureMul + EnsureDiv> EnsureOp for T {}

/// Performs self addition that returns [`ArithmeticError`] instead of wrapping around on
/// overflow.
pub trait EnsureAddAssign: EnsureAdd {
pub trait EnsureAddAssign: CheckedAdd + PartialOrd + Zero {
/// Adds two numbers overwriting the left hand one, checking for overflow.
///
/// If it fails, [`ArithmeticError`] is returned.
Expand All @@ -496,14 +496,14 @@ mod ensure {
/// assert_eq!(underflow(), Err(ArithmeticError::Underflow));
/// ```
fn ensure_add_assign(&mut self, v: Self) -> Result<(), ArithmeticError> {
*self = self.ensure_add(v)?;
*self = self.checked_add(&v).ok_or_else(|| error::equivalent(&v))?;
Ok(())
}
}

/// Performs self subtraction that returns [`ArithmeticError`] instead of wrapping around on
/// underflow.
pub trait EnsureSubAssign: EnsureSub {
pub trait EnsureSubAssign: CheckedSub + PartialOrd + Zero {
/// Subtracts two numbers overwriting the left hand one, checking for overflow.
///
/// If it fails, [`ArithmeticError`] is returned.
Expand All @@ -527,14 +527,14 @@ mod ensure {
/// assert_eq!(overflow(), Err(ArithmeticError::Overflow));
/// ```
fn ensure_sub_assign(&mut self, v: Self) -> Result<(), ArithmeticError> {
*self = self.ensure_sub(v)?;
self.checked_sub(&v).ok_or_else(|| error::inverse(&v))?;
Ok(())
}
}

/// Performs self multiplication that returns [`ArithmeticError`] instead of wrapping around on
/// overflow.
pub trait EnsureMulAssign: EnsureMul {
pub trait EnsureMulAssign: CheckedMul + PartialOrd + Zero {
/// Multiplies two numbers overwriting the left hand one, checking for overflow.
///
/// If it fails, [`ArithmeticError`] is returned.
Expand All @@ -558,14 +558,14 @@ mod ensure {
/// assert_eq!(underflow(), Err(ArithmeticError::Underflow));
/// ```
fn ensure_mul_assign(&mut self, v: Self) -> Result<(), ArithmeticError> {
*self = self.ensure_mul(v)?;
self.checked_mul(&v).ok_or_else(|| error::multiplication(self, &v))?;
Ok(())
}
}

/// Performs self division that returns [`ArithmeticError`] instead of wrapping around on
/// overflow.
pub trait EnsureDivAssign: EnsureDiv {
pub trait EnsureDivAssign: CheckedDiv + PartialOrd + Zero {
/// Divides two numbers overwriting the left hand one, checking for overflow.
///
/// If it fails, [`ArithmeticError`] is returned.
Expand All @@ -589,7 +589,7 @@ mod ensure {
/// assert_eq!(overflow(), Err(ArithmeticError::Overflow));
/// ```
fn ensure_div_assign(&mut self, v: Self) -> Result<(), ArithmeticError> {
*self = self.ensure_div(v)?;
self.checked_div(&v).ok_or_else(|| error::division(self, &v))?;
Ok(())
}
}
Expand All @@ -609,9 +609,8 @@ mod ensure {
{
}

/// Meta trait that supports all arithmetic operations
pub trait Ensure: EnsureOp + EnsureOpAssign {}
impl<T: EnsureOp + EnsureOpAssign> Ensure for T {}
impl<T: BaseArithmetic> Ensure for T {}

/// Extends [`FixedPointNumber`] with the Ensure family functions.
pub trait EnsureFixedPointNumber: FixedPointNumber {
Expand Down Expand Up @@ -643,7 +642,7 @@ mod ensure {
d: D,
) -> Result<Self, ArithmeticError> {
<Self as FixedPointNumber>::checked_from_rational(n, d)
.ok_or_else(|| error::division(n, d))
.ok_or_else(|| error::division(&n, &d))
}

/// Ensure multiplication for integer type `N`. Equal to `self * n`.
Expand All @@ -670,7 +669,7 @@ mod ensure {
/// assert_eq!(underflow(), Err(ArithmeticError::Underflow));
/// ```
fn ensure_mul_int<N: FixedPointOperand>(self, n: N) -> Result<N, ArithmeticError> {
self.checked_mul_int(n).ok_or_else(|| error::multiplication(self, n))
self.checked_mul_int(n).ok_or_else(|| error::multiplication(&self, &n))
}

/// Ensure division for integer type `N`. Equal to `self / d`.
Expand All @@ -697,16 +696,14 @@ mod ensure {
/// assert_eq!(overflow(), Err(ArithmeticError::Overflow));
/// ```
fn ensure_div_int<D: FixedPointOperand>(self, d: D) -> Result<D, ArithmeticError> {
self.checked_div_int(d).ok_or_else(|| error::division(self, d))
self.checked_div_int(d).ok_or_else(|| error::division(&self, &d))
}
}

impl<T: FixedPointNumber> EnsureFixedPointNumber for T {}

/// Similar to [`TryFrom`] but returning an [`ArithmeticError`] error.
pub trait EnsureFrom<T: PartialOrd + Zero + Copy>:
TryFrom<T> + PartialOrd + Zero + Copy
{
pub trait EnsureFrom<T: PartialOrd + Zero>: TryFrom<T> + PartialOrd + Zero {
/// Performs the conversion returning an [`ArithmeticError`] if fails.
///
/// Similar to [`TryFrom::try_from()`] but returning an [`ArithmeticError`] error.
Expand All @@ -728,14 +725,13 @@ mod ensure {
/// assert_eq!(underflow(), Err(ArithmeticError::Underflow));
/// ```
fn ensure_from(other: T) -> Result<Self, ArithmeticError> {
Self::try_from(other).map_err(|_| error::equivalent(other))
let err = error::equivalent(&other);
Copy link
Contributor

@lemunozm lemunozm Jan 2, 2023

Choose a reason for hiding this comment

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

Regarding the performance of this, it seems like the assembly resulting code of this can be easily reordered by the compiler and should not be penalty

Copy link
Member

@bkchr bkchr Jan 2, 2023

Choose a reason for hiding this comment

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

Did you check this?

Copy link
Contributor

@lemunozm lemunozm Jan 3, 2023

Choose a reason for hiding this comment

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

Not empirically (not sure how to do it well).

To extend my argument, error::equivalent has no side effects, and if the resulting value is only read under a condition, the compiler can only execute that for that condition. I understand that this works for any T that is not really consumed by the try_from(), which means that T is in fact a Copy type. The compiler could not optimize this if T is not Copy.

Copy link
Member

@bkchr bkchr Jan 3, 2023

Choose a reason for hiding this comment

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

Okay ;)

Self::try_from(other).map_err(|_| err)
}
}

/// Similar to [`TryInto`] but returning an [`ArithmeticError`] error.
pub trait EnsureInto<T: PartialOrd + Zero + Copy>:
TryInto<T> + PartialOrd + Zero + Copy
{
pub trait EnsureInto<T: PartialOrd + Zero>: TryInto<T> + PartialOrd + Zero {
/// Performs the conversion returning an [`ArithmeticError`] if fails.
///
/// Similar to [`TryInto::try_into()`] but returning an [`ArithmeticError`] error
Expand All @@ -757,12 +753,13 @@ mod ensure {
/// assert_eq!(underflow(), Err(ArithmeticError::Underflow));
/// ```
fn ensure_into(self) -> Result<T, ArithmeticError> {
self.try_into().map_err(|_| error::equivalent(self))
let err = error::equivalent(&self);
self.try_into().map_err(|_| err)
}
}

impl<T: TryFrom<S> + PartialOrd + Zero + Copy, S: PartialOrd + Zero + Copy> EnsureFrom<S> for T {}
impl<T: TryInto<S> + PartialOrd + Zero + Copy, S: PartialOrd + Zero + Copy> EnsureInto<S> for T {}
impl<T: TryFrom<S> + PartialOrd + Zero, S: PartialOrd + Zero> EnsureFrom<S> for T {}
impl<T: TryInto<S> + PartialOrd + Zero, S: PartialOrd + Zero> EnsureInto<S> for T {}

mod error {
use super::{ArithmeticError, Zero};
Expand All @@ -773,9 +770,9 @@ mod ensure {
Positive,
}

impl<T: PartialOrd + Zero> From<T> for Signum {
fn from(value: T) -> Self {
if value < Zero::zero() {
impl<T: PartialOrd + Zero> From<&T> for Signum {
fn from(value: &T) -> Self {
if value < &Zero::zero() {
Signum::Negative
} else {
Signum::Positive
Expand All @@ -795,33 +792,33 @@ mod ensure {
}
}

pub fn equivalent<R: PartialOrd + Zero + Copy>(r: R) -> ArithmeticError {
pub fn equivalent<R: PartialOrd + Zero>(r: &R) -> ArithmeticError {
match Signum::from(r) {
Signum::Negative => ArithmeticError::Underflow,
Signum::Positive => ArithmeticError::Overflow,
}
}

pub fn inverse<R: PartialOrd + Zero + Copy>(r: R) -> ArithmeticError {
pub fn inverse<R: PartialOrd + Zero>(r: &R) -> ArithmeticError {
match Signum::from(r) {
Signum::Negative => ArithmeticError::Overflow,
Signum::Positive => ArithmeticError::Underflow,
}
}

pub fn multiplication<L: PartialOrd + Zero + Copy, R: PartialOrd + Zero + Copy>(
l: L,
r: R,
pub fn multiplication<L: PartialOrd + Zero, R: PartialOrd + Zero>(
l: &L,
r: &R,
) -> ArithmeticError {
match Signum::from(l) * Signum::from(r) {
Signum::Negative => ArithmeticError::Underflow,
Signum::Positive => ArithmeticError::Overflow,
}
}

pub fn division<N: PartialOrd + Zero + Copy, D: PartialOrd + Zero + Copy>(
n: N,
d: D,
pub fn division<N: PartialOrd + Zero, D: PartialOrd + Zero>(
n: &N,
d: &D,
) -> ArithmeticError {
if d.is_zero() {
ArithmeticError::DivisionByZero
Expand Down