Consider taking `self` instead of `&self`

[calebzulawski]

I wanted to look into implementing these traits for Iterators, but taking &self makes that a little difficult. With the current traits there isn't a convenient way to implement the following blanket impl:

impl<T: FloatDiff, I: IntoIterator<Item=T>> FloatDiff for I {
    type AbsDiff = AbsDiffIter<T::AbsDiff>;

    fn abs_diff(&self, other: &Self) -> Self::AbsDiff {
        /* can't call self.into_iter() here */
    }

    /* similar for ulps_diff */
}

My proposed changes:

• Change traits to take self
• f32, f64, Complex remain unchanged since they are all Copy.
• Add blanket impls for IntoIterator, returning iterators over AbsDiff, etc.
• Change implementations for [T; N] to &[T; N] (if they are even still necessary, arrays implement IntoIterator)

Let me know what you think and if you'd like a little help implementing it, I didn't want to go change everything and just drop a PR without bringing it up :)

[jtempest]

Thank you for your interest!

I like the idea of being able to compare Iterators, but I would like to keep the parameter as &self. Conceptually, FloatEq mirrors the standard PartialEq, which means that it compares two borrowed values rather than consuming either of them. I would also really like to avoid requiring the input to be Copy-able, since it would be good to support comparing large values without requiring a copy or clone.

Is it possible to implement the existing trait for Iterator rather than IntoIterator to achieve what you're after? I can see the potential for an implementation over two different but compatible iterators as well, which sounds like it could be handy.

[calebzulawski]

I've thought about it a little more, and I understand where you're coming from, here are my thoughts:

• I think taking a shared reference prevents implementing over Iterators no matter what. IntoIterator requires a value and Iterator requires a mutable reference to advance.
• Taking self by value doesn't require the type to be Copy. For example, you could impl<'a> FloatDiff for &'a Foo. This is how some other traits, like std::ops::Add work.
• I'm not sure this is directly comparable to PartialEq. eq always returns bool, but FloatDiff, for example, may need to return something lifetime-bound, which I think is not possible without taking by value (or adding an explicit lifetime to the trait, but I don't think that's the right solution).

That said, I don't think returning an iterator is possible unless it is Copy or Clone, since Debug::fmt takes a shared reference. Each call to Debug::fmt would clone the iterator and print the contents.

I do want to point out that without changing the traits it would be possible to implement over slices, however due to the limitation I mentioned about lifetimes, the output would need to be allocated and wouldn't work in no_std.

[jtempest]

Oh! I must admit, I haven't thought that deeply about why PartialEq is defined by &self rather than as self. You've persuaded me that it might be worth changing as per your original suggestions :)

I think that my main potential worry is to do with the API becoming harder to use if different levels of borrowing start to conflict with one another at compile time (as this thread seems to suggest), but it's easy enough to write tests to cover those cases to check. The other thing is that I don't understand how these kinds of trait definitions might interact when they overlap yet, so that's also as much a learning exercise on my part too.

If you wished to investigate this yourself, I'd be happy to look at a PR, with the caveat that I'm new to using git/github so it might take me longer than you're used to! Otherwise, I can have a go at implementing it when I get the chance :)

[jtempest]

Right, I've done some more thinking/research and I have some further thoughts, my last reply may have been a bit hasty. I've not thought about the implications for Iterator implementations yet since I'm trying to get my head around the basics first. Apologies in advance, I'm also quite new to Rust in general.

• I'd like the traits to be idiomatic Rust unless there's a very good case for them not to be.
• Since I don't have much direct experience using traits like these yet, I'd like them to be patterned after how the standard library traits work, on the assumption that those are de facto idiomatic.

• Taking self by value doesn't require the type to be Copy. For example, you could impl<'a> FloatDiff for &'a Foo. This is how some other traits, like std::ops::Add work.

Makes sense, thank you for explaining. If I understand the implications correctly, binary operations using a non-borrowed self allow for the value to potentially be mutable, which would enable implementations over types like IntoIterator and optimisations where their value is mutated in place to then be used as the output, whereas using &self constrains you to always leave the original object intact and needing to allocate a new object to return (hence your next point and the one about slices). It seems that Serde agrees as well, in a somewhat different context.

• I'm not sure this is directly comparable to PartialEq. eq always returns bool, but FloatDiff, for example, may need to return something lifetime-bound, which I think is not possible without taking by value (or adding an explicit lifetime to the trait, but I don't think that's the right solution).

This also makes sense - I agree that FloatDiff is more of a binary op like Add rather than being like PartialEq, and it is intended to be used by itself as well as part of debugging output. Thus I think it might be worth looking into changing its method parameters to be by value (both self and other).

In terms of FloatEq, I think I'd still prefer to keep that as &self, following PartialEq. I think that taking self by value also necessitates the ne_* methods requiring that it be Sized, and I'm unsure of the implications of that as well.

The FloatEqDebug trait feels more like a binary op, but it also requires that FloatEq be implemented for its types. I think I'd rather leave that one as by &self since that's what FloatEq requires, and now that I think about it, the way that the asserts work using it probably precludes consuming IntoIterator and similar values as well, as the debug output requires the original values being compared to still exist.

Hopefully that makes sense?

[calebzulawski]

I completely agree that FloatEq can (and should) take &self.

You may be right about the implications in asserts (though shared references are Copy so it may work out). That might be a detail that works itself out after implementing something. Another option, if none of this seems to work particularly well, is a function (or iterator extension trait) and new set of asserts specifically for iterators. I realized that in the standard library, Iterators do not implement PartialEq, and instead have their own eq function which takes arguments by value.

[jtempest]

I've created an issue for making the change to FloatDiff. I'm going to leave FloatEq in terms of references, since I think that's more idiomatic.