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

RFC: Overloaded arithmetic and logical operators should take self and their arguments by value #118

Closed

Conversation

pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Jun 14, 2014

No description provided.


# Drawbacks

Some use cases of `+`, such as string and array concatentation, may become more verbose. It is likely that many of the use cases of `+` today will be compatible with these new semantics.
Copy link
Member

Choose a reason for hiding this comment

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

Fortunately we do neither of those with the + operator.

@ghost
Copy link

ghost commented Jun 14, 2014

This wouldn't work:

pub trait Add<RHS, Result> {
    fn add(self, rhs: RHS) -> Result;
}

fn double<A: Add<A, A>>(a: A) -> A {
    a + a // error: use of moved value: `a`
}

...and if you change the signature to:

fn double<A: Add<A, A> + Clone>(a: A) -> A {
    a.clone() + a
}

...then the function creates an unnecessary clone if the type of a owns its heap allocated memory and implements Add by taking the arguments to add by reference.

@matthieu-m
Copy link

@tommit I believe your usecase would be more efficiently addressed by supporting += or *= though.

@ghost
Copy link

ghost commented Jun 14, 2014

I think the solution to this issue should also cover any user-defined mathematical operators and other user-defined functions, like a trait for types that implement cross product.

@huonw
Copy link
Member

huonw commented Jun 14, 2014

It seems that this would stop types like BigInt being able to implement Num, since they would possibly be implementing something along the lines of Add<&BigInt, BigInt> for &BigInt, which doesn't match Nums Add<Self, Self> supertrait (at least, I assume this is the supertrait that would be used, since that's what works with the primitives).

@chris-morgan
Copy link
Member

This would alter the plans for augmented assignment significantly; the distinct traits would be rendered unnecessary.

As it is, the augmented assignment traits were still going to be taking the LHS by reference rather than by value.

I can see how this decreases complexity in some areas, especially as by-value LHS was going to be needed for augmented assignment at least and so it was going to be two traits to implement rather than one.

Still, I am not at all comfortable with this change; I certainly won’t go claiming it to be a Bad Thing™ as I simply don’t know that, but it will certainly lead to some surprises, e.g. that a BINOP a doesn’t work generally on generics (you’d need to add a Copy bound, which will exclude some types). Also that either side is consumed strikes me as significantly surprising with my primarily-Python background. How much trouble that may or may not cause is impossible to tell until it is done.

I agree that the functionality offered here is a superset of that offered with references (though presumably sans-autoref and thus uglier), but I question how much people will actually go implementing things for all the feasible combinations of things. I believe that the RHS will normally only need—or want, for that matter—a reference. Are we going to have people writing four (T + U, T + &U, &T + U, &T + &U) or at least two (T + T, &T + &T) implementations per pair of types? Or will we have implementations that end up with people needing to write &a + &b?

Yes, there are certainly concrete benefits from doing this. But there are also non-trivial risks (I won’t judge how great they are—at present I’m more interested in raising questions than answering them!) and it is a significant break from the traditions of all languages that I know (C++ is notably excluded from that set; I do not have any firm grasp on its operator overloading), one which is certain to lead to confusion in some people.

@Valloric
Copy link

Function overloading would solve a lot (all?) of the downsides of this approach. The compiler could choose the right overload. This is why move constructors work well in C++; they're picked over the copy constructor by the compiler because of the rvalue reference parameter (while the copy ctor has a plain const ref).

I'm not saying Rust needs move/copy ctors, but I am saying function overloading would solve problems here and elsewhere in Rust.

@pcwalton
Copy link
Contributor Author

That's not overloading. That's ad-hoc templating. + has no type signature in C++ and it is resolved only at the time it is used. One consequence of this design choice in C++ is that it is impossible to write a function that is generic over any two types that can be added via + without getting error messages at template instantiation time, rather than at the time the generic function is declared.

In general I am quite opposed to ad-hoc templates; I think strongly-typed generics are superior in pretty much all respects.

@Valloric
Copy link

@pcwalton I think you misunderstood what I was trying to say (I probably wasn't clear enough). I was trying to say we could have overloads for say add:

pub trait Add<RHS,Result> {
   fn add(self, rhs: RHS) -> Result;
   fn add(&self, rhs: RHS) -> Result;
}

You get the idea. The compiler could then pick the most appropriate one.

I am most certainly not advocating ad-hoc templating à la C++. We agree they are a terrible experience. Strongly-typed generics are a Rust selling point (other C++ devs drool when I mention them).

@anasazi
Copy link

anasazi commented Jun 16, 2014

@Valloric That's no different semantically or compiler-implementation-wise from simply implementing the proposed Add<RHS,Result> trait for both T and &T. Traits are function overloading.

@ghost ghost mentioned this pull request Jun 16, 2014
@Kimundi
Copy link
Member

Kimundi commented Jun 17, 2014

@anasazi There is a difference. In @Valloric's case, the Self type is the same for both overloaded methods, while implementing it for T and &T ends up with two different Self types.

@anasazi
Copy link

anasazi commented Jun 17, 2014

@Kimundi yeah, but that's not really important. The type of the receiver is the same in both cases (T and &T). The particular syntax for referring to those types (just Self for both in the trait version and Self/&Self in @Valloric's version) doesn't really matter. It's literally just a syntactic difference.

@seanmonstar
Copy link
Contributor

So, if I impl Add for &Foo, will it know to take a reference if I'm not
using one? let foo = Foo; foo + foo
On Jun 17, 2014 8:58 AM, "Eric Reed" [email protected] wrote:

@Kimundi https://github.com/Kimundi yeah, but that's not really
important. The type of the receiver is the same in both cases (T and &T).
The particular syntax for referring to those types (just Self for both in
the trait version and Self/&Self in @Valloric
https://github.com/Valloric's version) doesn't really matter. It's
literally just a syntactic difference.


Reply to this email directly or view it on GitHub
#118 (comment).

@sellibitze
Copy link

IMHO, this RFC needs a more thorough analysis w.r.t. its implications. So far, I'm not convinced that the benefits outweigh the costs.

Certain situations are improved (e.g. when values are only consumed once). In other situations the performance will be worse (e.g. If values are used more than once this requires explicit clone() to avoid "already moved"-errors. However, if the operator's implementation is not able to reuse the clones' buffers for the result but requires an out-of-place computation (only reading the arguments and computing the value in a new buffer), these clones would have been unnecessary! Think about BigInt multiplication here) And finally, it will make some expressions involving expensive types uglier due to clone().

I would also like to stress that the idea to impl these Traits for references &T instead of T à la Add<&T,T> for &T is a bad idea because it does not compose well with generic functions<T: Add<T,T>> since T would not be Add<T,T> in that case.

@pcwalton
Copy link
Contributor Author

@sellibitze This is a good point. If it doesn't help BigInt, then there isn't much of a point in doing it.

@Ericson2314
Copy link
Contributor

Once rust gets associated types, there could be a PreferedPassingStrategy<T> associated type, which would help with this and generics.

@alexchandel
Copy link

Doesn't this make addition/other operations more expensive? Since operands like BigInts, (algebraic) vectors, matrices, or N-dimensional arrays would have to be copied into a new stack whenever these are performed. Unless the implication is that these copies are guaranteed to be eliminated by the compiler.

@huonw
Copy link
Member

huonw commented Jul 5, 2014

@alexchandel most of those will be behind a pointer, e.g. BigInts are represented as Vec<u32>, and taking by value is just a shallow byte copy, i.e. just copying the pointer (and the small amount of metadata (e.g. length and capacity for Vec) it might happen to have).

@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 8, 2014

Closing because I don't like this anymore.

@pcwalton pcwalton closed this Jul 8, 2014
@ben0x539 ben0x539 mentioned this pull request Nov 5, 2014
withoutboats pushed a commit to withoutboats/rfcs that referenced this pull request Jan 15, 2017
wycats pushed a commit to wycats/rust-rfcs that referenced this pull request Mar 5, 2019
@dtolnay dtolnay changed the title RFC: Overloaded arithmetic and logical operators should take self and RFC: Overloaded arithmetic and logical operators should take self and their arguments by value Dec 13, 2022
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.