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

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

This comment has been minimized.

Copy link
Collaborator

commented Dec 2, 2014

warning Warning warning

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

@nikomatsakis nikomatsakis self-assigned this Dec 3, 2014

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2014

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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

commented Dec 4, 2014

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2014

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

@japaric japaric force-pushed the japaric:binops-by-value branch from 1589b18 to 065e409 Dec 6, 2014

@japaric

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2014

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

@aturon

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2014

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 japaric force-pushed the japaric:binops-by-value branch from 065e409 to 20218e6 Dec 13, 2014

@japaric

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2014

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

@japaric japaric force-pushed the japaric:binops-by-value branch from 20218e6 to 89d2061 Dec 14, 2014

@nikomatsakis

This comment has been minimized.

Copy link

commented on 89d2061 Dec 15, 2014

r+ p=1

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Dec 15, 2014

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

@bors

This comment has been minimized.

Copy link
Contributor

commented on 89d2061 Dec 15, 2014

saw approval from nikomatsakis
at japaric@89d2061

This comment has been minimized.

Copy link
Contributor

replied Dec 15, 2014

merging japaric/rust/binops-by-value = 89d2061 into auto

This comment has been minimized.

Copy link
Contributor

replied Dec 15, 2014

status: {"merge_sha": "0669a432a2e09ad08886cb2138dbe9f5d681fb7f"}

This comment has been minimized.

Copy link
Contributor

replied Dec 15, 2014

japaric/rust/binops-by-value = 89d2061 merged ok, testing candidate = 0669a43

This comment has been minimized.

Copy link
Contributor

replied Dec 16, 2014

fast-forwarding master to auto = 0669a43

This comment has been minimized.

Copy link
Contributor

replied Dec 16, 2014

fast-forwarding master to auto = 0669a43

bors added a commit that referenced this pull request Dec 15, 2014
auto merge of #19448 : japaric/rust/binops-by-value, r=nikomatsakis
- 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

2 checks passed

continuous-integration/travis-ci The Travis CI build passed
Details
default all tests passed

@japaric japaric deleted the japaric:binops-by-value branch Dec 16, 2014

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Dec 22, 2014
rollup merge of rust-lang#20125: csouth3/hashset-bitops
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
Projects
None yet
5 participants
You can’t perform that action at this time.