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

Operator overloading not flexible enough? #21188

Closed
SirVer opened this issue Jan 15, 2015 · 8 comments
Closed

Operator overloading not flexible enough? #21188

SirVer opened this issue Jan 15, 2015 · 8 comments
Labels
A-trait-system Area: Trait system

Comments

@SirVer
Copy link

SirVer commented Jan 15, 2015

I might just missing some informations here, but I have a hard time implementing a simple Point type that can be added:

use std::ops::{Add, Sub};

#[derive(Show)]
struct Point {
    x: f32,
    y: f32,
}

impl Add for Point {
    type Output = Point;
    fn add(self, other: Point) -> Point {
        Point{x: self.x + other.x, y: self.y + other.y}
    }
}

#[test]
fn point_to_point_addition() {
    let a = Point {x: 3f32, y: 5f32};
    let b = Point {x: -1f32, y: 5f32};
    let res = a + b;
    let res1 = b + a; // <- Error: use of moved value: `b` and `a`
}

That error makes sense to me, so I went ahead and tried implementing this for &'a Point.

impl<'a> Add for &'a Point {
    type Output = Point;
    fn add(self, other: &'a Point) -> Point {
        Point{x: self.x + other.x, y: self.y + other.y}
    }
}

#[test]
fn point_to_point_addition() {
    let a = &Point {x: 3f32, y: 5f32};
    let b = &Point {x: -1f32, y: 5f32};
    let res = a + b;
    let res1 = b + a;
    let res2 = a + b + b + a; // <- this now no longer works :(
}

But now chaining does no longer work since the Add returns a Point, not a &mut Point. Can somebody enlighten me about the correct way of solving this?

Another - maybe unrelated - issue I stumbled on is how to make Point + f32 do the same as f32 + Point.

@kmcallister kmcallister added the A-trait-system Area: Trait system label Jan 16, 2015
@AndyShiue
Copy link

I think #[derive(Copy)] for your Point solves the first problem?

@AndyShiue
Copy link

And it's impossible to implement both Point + f32 and f32 + Point right now, see #20749

@SirVer
Copy link
Author

SirVer commented Jan 16, 2015

I think #[derive(Copy)] for your Point solves the first problem?

Yes. But this is just an example - what about something that is expensive to clone, like a Mat100x100. But I think I just found the answer in the bigint implementation: http://doc.rust-lang.org/num/src/num/bigint.rs.html#234

You just have to implement all combinations of &ref and value.. uhh.

And I did still not figure out how you can define an overloaded operator for the RHS, i.e. f32 + Point yet. I read #20749 that it is just not possible???

@Gankra
Copy link
Contributor

Gankra commented Jan 21, 2015

CC @aturon

@sellibitze
Copy link
Contributor

The code

a + b + b + a

is equivalent to

((a + b) + b) + a

where the inner a+b is of type Point and b is of type &Point. So, there are different ways of solving this. Option 1:

&(&(a + b) + b) + a

This option is not really desirable in my opinion. Option 2 would be to add at least another implementation:

impl<'a> Add<&'a Point> for Point {}

(and possibly the other way around for symmetry).

Providing all these impls is a bit of pain in the butt. But I think macros can mitigate this to some extend. For example, we could try to standardize a macro for Copyable arithmetic types which automatically implement operators on references in terms of their value variant. I already did this in libcore/ops.rs. These macros are just not exposed as part of the public interface. Another macro that might be useful is to implement the value variants in terms of the implementations for references. This should be usedful for non-Copyable and more complicated types.

@aturon
Copy link
Member

aturon commented Feb 16, 2015

@nikomatsakis and I have been talking about ways to address this sort of thing by allowing you to write just the by-ref implementations of the operators and having the compiler automatically insert & for the operands in that case (as the traits used to do). Stay tuned for an RFC!

@huonw
Copy link
Member

huonw commented Jan 8, 2016

I think the scheme we've settled on here is to implement the operator traits for the 4 referenced/non-referenced combinations, i.e.

impl Add<Point> for Point { ... }
impl<'a> Add<Point> for &'a Point { ... }
impl<'a> Add<&'a Point> for Point { ... }
impl<'a> Add<&'a Point> for &'a Point { ... }

This is somewhat annoying, but for a simple/cheap type like Point 3 of those can just call the fourth (i.e. only one non-trivial implementation is needed). On the other hand, fancier types (e.g. big integers) are often be able to optimise certain combinations to, e.g., avoid copies and reuse allocations, meaning they'll want to write out all the combinations anyway.

I'm inclined to close this, despite us not having the auto-ref idea @aturon mentions, that is, one has to write something like &a + &b + &b + &a even with the impls I write above. That said, it seems the auto-ref would have to be pretty magical to handle a case like a + b + b + a without just falling back to the ref'd impl always (which would be a non-trivial perf hit for, e.g., big integers).

@aturon
Copy link
Member

aturon commented Jan 8, 2016

@huonw I have no objection to closing on that basis. I'd still love to have some for of auto-ref, but it's an RFC concern at this point.

@huonw huonw closed this as completed Jan 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-system Area: Trait system
Projects
None yet
Development

No branches or pull requests

7 participants