Skip to content

Change FloatDiff to take its arguments by value #2

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

Closed
jtempest opened this issue May 16, 2020 · 1 comment
Closed

Change FloatDiff to take its arguments by value #2

jtempest opened this issue May 16, 2020 · 1 comment

Comments

@jtempest
Copy link
Owner

jtempest commented May 16, 2020

The FloatDiff trait as of the 0.2 release is specified in a non-idiomatic fashion when compared to Rust's standard arithmetic operations, in particular std::ops::Sub, which it is analogous to. This makes it difficult to implement over iterable types. Additionally, since it takes other by reference, it is impossible to implement the following common pattern (see also forward_ref_binop), which can be important for certain optimisations:

impl Sub for MyType
impl Sub for &MyType
impl Sub<&MyTrait> for MyType
impl Sub<&MyTrait> for &MyType

The other consideration is whether FloatDiff ought to be two different traits, one per kind of difference op. Since its primary purpose in this library is to provide debug context information to assert_float_eq and friends, for the moment I'm content to leave it as a single trait, since it compels an implementor to provide all of the required information.

Therefore, the work to be done is that FloatDiff should be changed to the following:

pub trait FloatDiff<Rhs = Self> {
    type AbsDiff;
    type UlpsDiff;

    fn abs_diff(self, other: Rhs) -> Self::AbsDiff;
    fn ulps_diff(self, other: Rhs) -> Self::UlpsDiff;
}

This would necessitate changing the existing implementations (f32, f64, Complex and [T; n]) to take this new form. As part of this change, I'd also like to implement the trait across LHS and RHS references for those types (as Sub is over primitive types).

@jtempest jtempest changed the title Split FloatDiff into FloatAbsDiff and FloatUlpsDiff Change FloatDiff to take its arguments by value May 16, 2020
@jtempest
Copy link
Owner Author

The assert macros which use FloatDiff require it to use reference versions without significantly more documentation/expectation on the user. That means that most often FloatDiff<&A> &B would be the implemented version to enable assertions, which isn't significantly different to the current formulation, but provides more potential pitfalls/onus on the implementor. Since FloatDiff is entirely used by the library for debug info, I'm going to leave it as-is. If the library was more general, it would be a more pressing change.

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

No branches or pull requests

1 participant