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

Overloaded augmented assignments #28345

Merged
merged 2 commits into from
Sep 19, 2015
Merged

Overloaded augmented assignments #28345

merged 2 commits into from
Sep 19, 2015

Conversation

japaric
Copy link
Member

@japaric japaric commented Sep 11, 2015

Implements overload-able augmented/compound assignments, like a += b via the AddAssign trait, as specified in RFC 953

r? @nikomatsakis

@bors
Copy link
Contributor

bors commented Sep 14, 2015

☔ The latest upstream changes (presumably #28358) made this pull request unmergeable. Please resolve the merge conflicts.

@@ -167,14 +155,16 @@ fn check_overloaded_binop<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
lhs_expr: &'tcx hir::Expr,
lhs_ty: Ty<'tcx>,
rhs_expr: &'tcx hir::Expr,
op: hir::BinOp)
op: hir::BinOp,
assign: bool)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can we newtype this bool? I think I prefer something like:

struct IsAssign(bool)

and then in the call foo(..., IsAssign(true)), though IsAssign::Yes is also ok.

@nikomatsakis
Copy link
Contributor

This looks good. r+ modulo nits -- can you also add a test that you can't implement AddAssign and friends without the suitable feature gate? (Another related question -- do we really want two distinct feature gates? Seems like the traits and associated syntax live or die together?)

Also: I think it may be worth waiting to land this until the beta is released, just in case it introduces accidental breakage.

@japaric
Copy link
Member Author

japaric commented Sep 17, 2015

@nikomatsakis I've addressed all your comments (I think)

do we really want two distinct feature gates? Seems like the traits and associated syntax live or die together?

I did try using the same feature name for the language feature and for the traits, but it didn't work. What I observed is that when I added #![feature(augmented_assignment)] to some source file, the language feature was enabled, i.e. a += b didn't raised an error; but implementing the OpAssign trait still raised the "unstable library feature 'augmented_assignments'" error. Which seems to indicate there's a bug when a language feature and a library feature have the same name: #![feature(foo)] enables the language feature foo, but not the library feature foo when it should enable both.

Also: I think it may be worth waiting to land this until the beta is released, just in case it introduces accidental breakage.

Sounds good to me

@bors
Copy link
Contributor

bors commented Sep 18, 2015

☔ The latest upstream changes (presumably #28336) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

@japaric ok feel free to r=me once rebased

@japaric
Copy link
Member Author

japaric commented Sep 19, 2015

@bors r=nmatsakis

@bors
Copy link
Contributor

bors commented Sep 19, 2015

📌 Commit f5569ec has been approved by nmatsakis

@bors
Copy link
Contributor

bors commented Sep 19, 2015

⌛ Testing commit f5569ec with merge c61bf60...

@bors
Copy link
Contributor

bors commented Sep 19, 2015

💔 Test failed - auto-linux-64-opt

@bluss
Copy link
Member

bluss commented Sep 19, 2015

@bors retry

@bors
Copy link
Contributor

bors commented Sep 19, 2015

⌛ Testing commit f5569ec with merge 295d51f...

@bors
Copy link
Contributor

bors commented Sep 19, 2015

💔 Test failed - auto-linux-64-opt

@alexcrichton
Copy link
Member

@bors: retry

On Sat, Sep 19, 2015 at 4:08 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-linux-64-opt
http://buildbot.rust-lang.org/builders/auto-linux-64-opt/builds/6468


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

bors added a commit that referenced this pull request Sep 19, 2015
Implements overload-able augmented/compound assignments, like `a += b` via the `AddAssign` trait, as specified in RFC [953]

[953]: https://github.com/rust-lang/rfcs/blob/master/text/0953-op-assign.md

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Sep 19, 2015

⌛ Testing commit f5569ec with merge 783c3fc...

@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 19, 2015
@japaric japaric deleted the op-assign branch September 20, 2015 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants