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

Make binary operators take their arguments by value #19448

Merged
merged 30 commits into from
Dec 16, 2014

Conversation

japaric
Copy link
Member

@japaric japaric commented Dec 2, 2014

  • 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

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@nikomatsakis nikomatsakis self-assigned this Dec 3, 2014
@nikomatsakis
Copy link
Contributor

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.

@japaric
Copy link
Member Author

japaric commented Dec 3, 2014

@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 Vec/String addition (which I missed in my initial PR)

@aturon
Copy link
Member

aturon commented Dec 4, 2014

I've reviewed the library changes and am happy with them. Great work as always.

@nikomatsakis
Copy link
Contributor

@japaric this looks great, let's discuss how/when to land it tomorrow

@japaric
Copy link
Member Author

japaric commented Dec 6, 2014

Rebased, and added commit converting the new TrieSet binops (#19518) to by-value.

@aturon
Copy link
Member

aturon commented Dec 8, 2014

@japaric A quick update -- I think that @nikomatsakis is getting closer on generalized where clauses, which would allow us to make this change without even a temporary regression in expressivity.

I'll let him decide on the exact staging, however.

@nikomatsakis
Copy link
Contributor

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.

@japaric
Copy link
Member Author

japaric commented Dec 13, 2014

@nikomatsakis Rebased, had to convert some new binops implementations for BTreeSet and TreeSet to by-value (see last 2 commits).

@nikomatsakis
Copy link
Contributor

Giving p=1 because this is a crucial backwards compat change.

bors added a commit that referenced this pull request Dec 15, 2014
- 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
@bors bors merged commit 89d2061 into rust-lang:master Dec 16, 2014
@japaric japaric deleted the binops-by-value branch December 16, 2014 00:50
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Dec 22, 2014
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.
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.

5 participants