-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Make binary operators take their arguments by value #19448
Conversation
So, I read the patch over and it looks good. I am ready to r+, however I want to be a bit careful about when we land this, because it's a breaking change that, without generalized where clauses, can't always be worked around in a simple way. |
@nikomatsakis Addressed all you comments (I think!), and added a new compile-fail test for move semantics, let me know if you have any other test case about that. @aturon Added the commutative versions of |
I've reviewed the library changes and am happy with them. Great work as always. |
@japaric this looks great, let's discuss how/when to land it tomorrow |
1589b18
to
065e409
Compare
Rebased, and added commit converting the new |
@japaric A quick update -- I think that @nikomatsakis is getting closer on generalized I'll let him decide on the exact staging, however. |
My opinion is we should just land this as quickly as we can; we'll bring in the generalized where clauses as soon as they are ready to. I'll do a final review this weekend / monday. |
065e409
to
20218e6
Compare
@nikomatsakis Rebased, had to convert some new binops implementations for |
20218e6
to
89d2061
Compare
Giving p=1 because this is a crucial backwards compat change. |
- The following operator traits now take their arguments by value: `Add`, `Sub`, `Mul`, `Div`, `Rem`, `BitAnd`, `BitOr`, `BitXor`, `Shl`, `Shr`. This breaks all existing implementations of these traits. - The binary operation `a OP b` now "desugars" to `OpTrait::op_method(a, b)` and consumes both arguments. - `String` and `Vec` addition have been changed to reuse the LHS owned value, and to avoid internal cloning. Only the following asymmetric operations are available: `String + &str` and `Vec<T> + &[T]`, which are now a short-hand for the "append" operation. [breaking-change] --- This passes `make check` locally. I haven't touch the unary operators in this PR, but converting them to by value should be very similar to this PR. I can work on them after this gets the thumbs up. @nikomatsakis r? the compiler changes @aturon r? the library changes. I think the only controversial bit is the semantic change of the `Vec`/`String` `Add` implementation. cc #19148
Now that rust-lang#19448 has landed in a snapshot, we can add proper by-value operator overloads for `HashSet`. The behavior of these operator overloads is consistent with rust-lang/rfcs#235.
Add
,Sub
,Mul
,Div
,Rem
,BitAnd
,BitOr
,BitXor
,Shl
,Shr
. This breaks all existing implementations of these traits.a OP b
now "desugars" toOpTrait::op_method(a, b)
and consumes both arguments.String
andVec
addition have been changed to reuse the LHS owned value, and to avoid internal cloning. Only the following asymmetric operations are available:String + &str
andVec<T> + &[T]
, which are now a short-hand for the "append" operation.[breaking-change]
This passes
make check
locally. I haven't touch the unary operators in this PR, but converting them to by value should be very similar to this PR. I can work on them after this gets the thumbs up.@nikomatsakis r? the compiler changes
@aturon r? the library changes. I think the only controversial bit is the semantic change of the
Vec
/String
Add
implementation.cc #19148