Skip to content

Conversation

japaric
Copy link
Contributor

@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
Collaborator

bors commented Sep 14, 2015

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

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
Contributor 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
Collaborator

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
Contributor Author

japaric commented Sep 19, 2015

@bors r=nmatsakis

@bors
Copy link
Collaborator

bors commented Sep 19, 2015

📌 Commit f5569ec has been approved by nmatsakis

@bors
Copy link
Collaborator

bors commented Sep 19, 2015

⌛ Testing commit f5569ec with merge c61bf60...

@bors
Copy link
Collaborator

bors commented Sep 19, 2015

💔 Test failed - auto-linux-64-opt

@bluss
Copy link
Contributor

bluss commented Sep 19, 2015

@bors retry

@bors
Copy link
Collaborator

bors commented Sep 19, 2015

⌛ Testing commit f5569ec with merge 295d51f...

@bors
Copy link
Collaborator

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
Collaborator

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