Add OpAssign to Wrapping<T>, etc. in core::num::wrapping by strega-nil · Pull Request #30523 · rust-lang/rust

strega-nil · 2015-12-22T12:04:27Z

Add OpAssign to Wrapping, plus fix some problems in core::num::wrapping

including, but not limited to:

Testing Wrapping
Pull out a lot of broken code that doesn't need to be there with the new stage0 compiler
Adding Rem and RemAssign to Wrapping
Removed 3 (assumed accidental) re-exports, which is a minor [breaking-change].
Change shl and shr to take all integer types, instead of a usize; this is a more major [breaking-change], because of values that were inferred before, but brings us in line with the integer shifts.

Fixes #30524 and #30523

rust-highfive · 2015-12-22T12:04:41Z

r? @alexcrichton

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

petrochenkov · 2015-12-22T12:10:01Z

Isn't AddAssign available in the new snapshot?

ah, right, I was developing this on the old snapshot by accident.

bluss · 2015-12-22T12:26:52Z

The rest of the PR looks fine to me. Please rename the PR to indicate that the main thing is adding the OpAssign implementations. Also note that the PR title is not included in bors' merge log, so I recommend just copying the title line to the first line of the PR message too.

bluss · 2015-12-22T12:31:14Z

Thanks! Note that the discussion in #27755 is not done yet, but it will recommend removing OverflowingOps as a trait. Touching it here doesn't change much, so it seems fine.

alexcrichton · 2015-12-22T17:48:10Z

Thanks, looks good to me! Could you also squash the commits into one?

bluss · 2015-12-22T18:31:18Z

This needs the [breaking-change] tag in the commit log for the removal of pub on the functions in std::num::wrapping.

sfackler · 2015-12-27T15:20:15Z

Is this good to go?

strega-nil · 2015-12-27T16:21:26Z

@sfackler sorry, almost... I wanted to add some more impls.

strega-nil · 2015-12-27T16:23:56Z

@sfackler unless you can think of anything more, I think it's done now, although obviously it needs a rebase.

bluss · 2015-12-30T13:25:06Z

The Wrapping<T> + T operations are fine with me, but what does the libs team think? (If there's been discussion already I must have missed it).

glaebhoerl · 2015-12-30T15:53:59Z

I suggested the same thing here and people raised some valid points in response.

strega-nil · 2015-12-31T01:23:20Z

@bluss @glaebhoerl We don't impl T + Wrapping<T>, only Wrapping<T> + T. The issue is one of ergonomics, and there's no arguing that Wrapping<T> + T should not output a Wrapping<T> and wrap. Our current way of doing things is awful; we need to fix it.

eddyb · 2016-01-01T02:31:25Z

@bors r+

bors · 2016-01-01T02:31:25Z

📌 Commit 165f5da has been approved by eddyb

bors · 2016-01-01T02:31:27Z

⌛ Testing commit 165f5da with merge 68d1928...

bors · 2016-01-01T03:14:38Z

💔 Test failed - auto-mac-64-nopt-t

strega-nil · 2016-01-01T03:34:18Z

@bors @eddyb I found a bug with these new tests! :D

strega-nil · 2016-01-02T06:25:05Z

@sfackler No, because we need to handle differently sized and signed types.

glaebhoerl · 2016-01-02T14:27:26Z

@ubsan I'm not sure if I'm interpreting your next-to-last sentence right - are you saying type Output = Wrapping<T> is good, bad but preferable to the alternatives, something else...?

I don't have a strong opinion about this myself either way, but process-wise it feels wrong that there were concerns raised about this the last time it was suggested, and now it's just being added (and impl fors are insta-stable, right?). Things which are controversial shouldn't just slip through, I think.

(cc @llogiq @rkjnsn)

llogiq · 2016-01-02T15:41:19Z

@glaebhoerl thanks for the ping. I'm not sure I understand the issue. Will have to wait until I get to my PC.

bluss · 2016-01-02T16:54:16Z

Yes, we need to have a discussion about the mixed operands.

strega-nil · 2016-01-02T22:49:10Z

@glaebhoerl @llogiq I... just realized that Wrapping<T> is not unstable. I have no idea why I thought that. I'm going to do a different PR for the Add(Assign)?<T> for Wrapping<T>, so that there can be discussion there about it.

eddyb · 2016-01-04T05:23:16Z

@bors r+

bors · 2016-01-04T05:23:16Z

📌 Commit 402259d has been approved by eddyb

Add OpAssign to Wrapping<T>, plus fix some problems in core::num::wrapping including, but not limited to: * Testing Wrapping<T> * Pull out a lot of broken code that doesn't need to be there with the new stage0 compiler * Adding Rem and RemAssign to Wrapping<T> * Removed 3 (assumed accidental) re-exports, which is a minor [breaking-change]. * Change shl and shr to take all integer types, instead of a usize; this is a more major [breaking-change], because of values that were inferred before, but brings us in line with the integer shifts. Fixes #30524 and #30523

bors · 2016-01-04T07:45:22Z

⌛ Testing commit 402259d with merge 50726c1...

bors · 2016-01-04T09:01:56Z

💔 Test failed - auto-mac-64-opt

glaebhoerl · 2016-01-04T15:02:54Z

FWIW I don't think Wrapping<T> += T is problematic -- there's really only one way it could behave and that behavior doesn't seem the least bit surprising -- but I'll let others weigh in (and there's no harm in delaying it, either).

bluss · 2016-01-04T18:19:47Z

@bors retry

bors · 2016-01-04T18:37:22Z

⌛ Testing commit 402259d with merge 5e8cb38...

Add OpAssign to Wrapping<T>, plus fix some problems in core::num::wrapping including, but not limited to: * Testing Wrapping<T> * Pull out a lot of broken code that doesn't need to be there with the new stage0 compiler * Adding Rem and RemAssign to Wrapping<T> * Removed 3 (assumed accidental) re-exports, which is a minor [breaking-change]. * Change shl and shr to take all integer types, instead of a usize; this is a more major [breaking-change], because of values that were inferred before, but brings us in line with the integer shifts. Fixes #30524 and #30523

bors · 2016-01-04T20:24:33Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

Fix a breaking change in #30523 While this does fix a breaking change, it is also, technically, a [breaking-change] to go back to our original way

rust-highfive assigned alexcrichton Dec 22, 2015

petrochenkov reviewed Dec 22, 2015
View reviewed changes

strega-nil changed the title ~~Fix some core::num::wrapping problems~~ Add OpAssign to Wrapping<T>, etc. in core::num::wrapping Dec 22, 2015

bluss mentioned this pull request Dec 25, 2015

Implement arithmetic assignment ops for Wrapping #30554

Closed

In the middle of the implementation

f962432

Finish test implementation

cd3aa31

Take out Op<T>/OpAssign<T> for Wrapping<T>

402259d

bors merged commit 402259d into rust-lang:master Jan 4, 2016

SimonSapin mentioned this pull request Jan 5, 2016

Type inference(?) regression, rand fails to build on nightly 2016-01-04 #30713

Closed

strega-nil mentioned this pull request Jan 6, 2016

Fix a breaking change in #30523 #30733

Merged

bors added a commit that referenced this pull request Jan 6, 2016

Auto merge of #30733 - ubsan:wrapping_op_assign, r=eddyb

21b025f

Fix a breaking change in #30523 While this does fix a breaking change, it is also, technically, a [breaking-change] to go back to our original way

Uh oh!

Uh oh!

Conversation

strega-nil commented Dec 22, 2015

Uh oh!

rust-highfive commented Dec 22, 2015

Uh oh!

petrochenkov Dec 22, 2015

Choose a reason for hiding this comment

Uh oh!

strega-nil Dec 22, 2015

Choose a reason for hiding this comment

Uh oh!

bluss commented Dec 22, 2015

Uh oh!

bluss commented Dec 22, 2015

Uh oh!

alexcrichton commented Dec 22, 2015

Uh oh!

bluss commented Dec 22, 2015

Uh oh!

sfackler commented Dec 27, 2015

Uh oh!

strega-nil commented Dec 27, 2015

Uh oh!

strega-nil commented Dec 27, 2015

Uh oh!

bluss commented Dec 30, 2015

Uh oh!

glaebhoerl commented Dec 30, 2015

Uh oh!

strega-nil commented Dec 31, 2015

Uh oh!

eddyb commented Jan 1, 2016

Uh oh!

bors commented Jan 1, 2016

Uh oh!

bors commented Jan 1, 2016

Uh oh!

bors commented Jan 1, 2016

Uh oh!

strega-nil commented Jan 1, 2016

Uh oh!

strega-nil commented Jan 2, 2016

Uh oh!

glaebhoerl commented Jan 2, 2016

Uh oh!

llogiq commented Jan 2, 2016

Uh oh!

bluss commented Jan 2, 2016

Uh oh!

strega-nil commented Jan 2, 2016

Uh oh!

eddyb commented Jan 4, 2016

Uh oh!

bors commented Jan 4, 2016

Uh oh!

bors commented Jan 4, 2016

Uh oh!

bors commented Jan 4, 2016

Uh oh!

glaebhoerl commented Jan 4, 2016

Uh oh!

bluss commented Jan 4, 2016

Uh oh!

bors commented Jan 4, 2016

Uh oh!

bors commented Jan 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants