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.

Sign up for GitHub

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

Jump to bottom

Implement cmp and ops reform #19148

Closed
alexcrichton opened this issue Nov 20, 2014 · 13 comments · Fixed by #21258
Closed

Implement cmp and ops reform #19148

alexcrichton opened this issue Nov 20, 2014 · 13 comments · Fixed by #21258
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented.
Milestone
1.0 beta

Comments

@alexcrichton
Copy link
Member

Tracking issue for rust-lang/rfcs#439
@alexcrichton alexcrichton added the B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. label Nov 20, 2014
@alexcrichton alexcrichton mentioned this issue Nov 20, 2014
Merged
@Gankra
Copy link
Contributor

Gankra commented Nov 20, 2014

What's the implementation plan?

@japaric
Copy link
Member

japaric commented Nov 20, 2014

I'll look into the comparison traits, which seems like one of the few things currently actionable. (Anything that needs associated types is blocked on #18594, #19129, etc)

@alexcrichton
Copy link
Member Author

@gankro I think that @japaric is right in that the entire RFC isn't necessarily actionable right now, but I do think that we should try to implement as much as possible in the meantime. Just knowing the list of blockers would be helpful!

@aturon
Copy link
Member

aturon commented Nov 20, 2014

Nominating P-backcompat-lang 1.0 (though we may want to triage parts of this separately.)

@japaric japaric mentioned this issue Nov 21, 2014
Merged
@japaric
Copy link
Member

japaric commented Nov 21, 2014

I think that another actionable item is changing Add et al. from by-ref to by-value.

@aturon
Copy link
Member

aturon commented Nov 21, 2014

@japaric

I think that another actionable item is changing Add et al. from by-ref to by-value.

Agreed. That will likely need to be coordinated with language changes, since currently & is implicitly added to the arguments. @nikomatsakis was working recently in this area and could probably help guide the staging.

@nikomatsakis
Copy link
Contributor

On Fri, Nov 21, 2014 at 08:53:05AM -0800, Aaron Turon wrote:

Agreed. That will likely need to be coordinated with language
changes, since currently & is implicitly added to the
arguments. @nikomatsakis was working recently in this area and could
probably help guide the staging.

Hmm. Yes, ping me, or I can try to get started on this. Sorry for the
delay, way behind on github mail.

@japaric
Copy link
Member

japaric commented Dec 1, 2014

I think that another actionable item is changing Add et al. from by-ref to by-value.

I'll look into this during this week. I'm now somewhat familiar with this part of the compiler, and this looks doable now. I'll ping @nikomatsakis on IRC if I find any issue that I can't sort out.

@japaric japaric mentioned this issue Dec 2, 2014
Merged
bors added a commit that referenced this issue Dec 4, 2014
@bors
auto merge of #19167 : japaric/rust/rhs-cmp, r=aturon
6d965cc 
Comparison traits have gained an `Rhs` input parameter that defaults to `Self`. And now the comparison operators can be overloaded to work between different types. In particular, this PR allows the following operations (and their commutative versions):

- `&str` == `String` == `CowString`
- `&[A]` == `&mut [B]` == `Vec<C>` == `CowVec<D>` == `[E, ..N]` (for `N` up to 32)
- `&mut A` == `&B` (for `Sized` `A` and `B`)

Where `A`, `B`, `C`, `D`, `E` may be different types that implement `PartialEq`. For example, these comparisons are now valid: `string == "foo"`, and `vec_of_strings == ["Hello", "world"]`.

[breaking-change]s

Since the `==` may now work on different types, operations that relied on the old "same type restriction" to drive type inference, will need to be type annotated. These are the most common fallout cases:

- `some_vec == some_iter.collect()`: `collect` needs to be type annotated: `collect::<Vec<_>>()`
- `slice == &[a, b, c]`: RHS doesn't get coerced to an slice, use an array instead `[a, b, c]`
- `lhs == []`: Change expression to `lhs.is_empty()`
- `lhs == some_generic_function()`: Type annotate the RHS as necessary

cc #19148

r? @aturon
@brson brson added this to the 1.0 milestone Dec 11, 2014
This was referenced Dec 13, 2014
Closed
Closed
@nrc
Copy link
Member

nrc commented Dec 13, 2014

Dealing with the range/slicing issues in #19794 and #19795.

bors added a commit that referenced this issue Dec 15, 2014
@bors
auto merge of #19448 : japaric/rust/binops-by-value, r=nikomatsakis
0669a43 
- 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
@japaric japaric mentioned this issue Jan 3, 2015
Closed
@AndyShiue
Copy link

All done?

@japaric
Copy link
Member

japaric commented Jan 4, 2015

@AndyShiue from the top of my head, as per the RFC the index(&self, &Index) method needs to take the Index parameter by value instead of by reference. And @nick29581 is unifying the slicing/indexing operators under the Index trait.

@aturon
Copy link
Member

aturon commented Jan 8, 2015

Nominating to move to -beta milestone.

@pnkfelix
Copy link
Member

pnkfelix commented Jan 8, 2015

Assigning 1.0-beta milestone.

@pnkfelix pnkfelix modified the milestones: 1.0 beta, 1.0 Jan 8, 2015
@alexcrichton alexcrichton mentioned this issue Jan 16, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented.
Projects
None yet
Milestone
1.0 beta
Development

Successfully merging a pull request may close this issue.

Stabilize Index traits and most range notation aturon/rust
9 participants
@alexcrichton @brson @nikomatsakis @pnkfelix @aturon @nrc @Gankra @japaric @AndyShiue