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

Add impls for `&Wrapping`. Also `Sum`, `Product` impls for both `Wrapping` and `&Wrapping`. #37356

Merged
merged 2 commits into from Nov 5, 2016

Conversation

Projects
None yet
7 participants
@cristicbz
Copy link
Contributor

cristicbz commented Oct 22, 2016

There are two changes here (split into two commits):

  • Ops for references to &Wrapping (Add, Sub, Mul etc.) similar to the way they are implemented for primitives.
  • Impls for iter::{Sum,Product} for Wrapping.

As far as I know impl stability attributes don't really matter so I didn't bother breaking up the macro for two different kinds of stability. Happy to change if it does matter.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Oct 22, 2016

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Oct 23, 2016

Looks good to me.

@cristicbz cristicbz force-pushed the cristicbz:wrapsum branch from 73c7dc3 to df0e5a9 Oct 23, 2016

@cristicbz cristicbz changed the title Add `Sum` and `Product` impls for `Wrapping` Add impls for `&Wrapping`. Also `Sum`, `Product` impls for both `Wrapping` and `&Wrapping`. Oct 23, 2016


// implements the unary operator "op &T"
// based on "op T" where T is expected to be `Copy`able
macro_rules! forward_ref_unop {

This comment has been minimized.

@cristicbz

cristicbz Oct 23, 2016

Contributor

These are moved from std::ops so that they can be shared for the Wrapping implementations.

}
}

#[stable(feature = "iter_arith_traits", since = "1.12.0")]
impl<'a> Product<&'a $a> for $a {
fn product<I: Iterator<Item=&'a $a>>(iter: I) -> $a {
iter.cloned().fold(1, Mul::mul)

This comment has been minimized.

@cristicbz

cristicbz Oct 23, 2016

Contributor

The cloned stuff is not necessary (and wasn't before) since Add::add etc are implemented for references of primitives (as well as, now, Wrapping).

@cristicbz

This comment has been minimized.

Copy link
Contributor

cristicbz commented Oct 23, 2016

@bluss I added a different commit to implement ops on Wrapping references. This also meant I could get rid of the cloned call in the Iterator::{sum,product} implementations. I'd appreciate it if you could have another look, thanks!

(I can split this into two PRs, but I thought getting rid of the cloned calls was worth merging the two changes).

@alexcrichton alexcrichton added the T-libs label Oct 25, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Oct 25, 2016

@rfcbot fcp merge

Looks good to me!

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Oct 25, 2016

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@brson brson added the relnotes label Nov 1, 2016

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Nov 3, 2016

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @alexcrichton, I wasn't able to add the final-comment-period label, please do so.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Nov 3, 2016

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 3, 2016

📌 Commit df0e5a9 has been approved by alexcrichton

jonathandturner added a commit to jonathandturner/rust that referenced this pull request Nov 4, 2016

Rollup merge of rust-lang#37356 - cristicbz:wrapsum, r=alexcrichton
Add impls for `&Wrapping`. Also `Sum`, `Product` impls for both `Wrapping` and `&Wrapping`.

There are two changes here (split into two commits):
- Ops for references to `&Wrapping`  (`Add`, `Sub`, `Mul` etc.) similar to the way they are implemented for primitives.
- Impls for `iter::{Sum,Product}` for `Wrapping`.

As far as I know `impl` stability attributes don't really matter so I didn't bother breaking up the macro for two different kinds of stability. Happy to change if it does matter.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 4, 2016

⌛️ Testing commit df0e5a9 with merge 713a360...

bors added a commit that referenced this pull request Nov 4, 2016

Auto merge of #37356 - cristicbz:wrapsum, r=alexcrichton
Add impls for `&Wrapping`. Also `Sum`, `Product` impls for both `Wrapping` and `&Wrapping`.

There are two changes here (split into two commits):
- Ops for references to `&Wrapping`  (`Add`, `Sub`, `Mul` etc.) similar to the way they are implemented for primitives.
- Impls for `iter::{Sum,Product}` for `Wrapping`.

As far as I know `impl` stability attributes don't really matter so I didn't bother breaking up the macro for two different kinds of stability. Happy to change if it does matter.

@bors bors merged commit df0e5a9 into rust-lang:master Nov 5, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment