Inherit overflow checks for sum and product #36372
Conversation
| fn sum<I: Iterator<Item=$a>>(iter: I) -> $a { | ||
| iter.fold(0, |a, b| { | ||
| a.checked_add(b).expect("overflow in sum") | ||
| Add::add(a, b) |
There was a problem hiding this comment.
This is not what I meant, although it would work (even without any added rustc_inherit_overflow_checks).
The intention was to replace the closure, i.e. iter.fold(0, Add::add).
There was a problem hiding this comment.
I seem to remember that at one point that syntax decayed to function pointers - is that not the case anymore?
There was a problem hiding this comment.
That hasn't been the case since some time before 1.0, at the type level, and since March or so this year as far as the low-level representation and optimizations go.
sfackler
force-pushed
the
sum-prod-overflow
branch
from
57e7c78 to
63fecad
Compare
| macro_rules! integer_sum_product { | ||
| ($($a:ident)*) => ($( | ||
| #[stable(feature = "iter_arith_traits", since = "1.12.0")] | ||
| impl Sum for $a { | ||
| #[rustc_inherit_overflow_checks] |
There was a problem hiding this comment.
Pretty sure you can remove the attribute now (not that it ever worked, AFAICT).
There was a problem hiding this comment.
Oops, yeah, just missed this one.
|
Is this something we actually want to guarantee? I thought MIR made it so that we couldn't do this and there's a "weird hack" to get it to work in some circumstances that we want to remove at some point. |
alexcrichton
added
the
T-libs-api
|
@alexcrichton The only way to remove the hack IMO is to have separate builds of libstd, with and without debug assertions. Either way the version of sum and product here won't change behavior if such a change occurs. |
|
@eddyb could you clarify what you mean by "won't change behavior if such a change occurs"? I was under the impression that we don't want to proliferate the semantics of |
|
@alexcrichton This PR (except for a line that should've been removed) doesn't need the attribute and solely relies on the ops traits (Add and Mul) for builtin types having the right semantics. If this was a larger patch I'd agree, but I don't see how this one would pose a problem. |
|
Right yeah this'll be sufficient without propagating the attributes themselves, but it propagates the semantics which we're unsure about. I was personally under the impression that #35807 is already a closed bug in that the semantics are exactly as documented, the iterator adaptors panic on overflow. We can and probably should discuss in @rust-lang/libs though! |
|
Discussed at libs triage the decision was to merge. @sfackler could you remove the attribute here and I'll merge? The point which swayed me was that the actual interface to this, |
|
Updated |
|
@bors: r+ |
|
📌 Commit 7bd25a3 has been approved by |
sfackler
added
the
relnotes
|
⌛ Testing commit 7bd25a3 with merge 140fbfc... |
|
💔 Test failed - auto-win-gnu-32-opt-rustbuild |
|
@bors: retry On Tue, Sep 13, 2016 at 7:22 PM, bors notifications@github.com wrote:
|
Inherit overflow checks for sum and product We have previously documented the fact that these will panic on overflow, but I think this behavior is what people actually want/expect. `#[rustc_inherit_overflow_checks]` didn't exist when we discussed these for stabilization. r? @alexcrichton Closes #35807
BTW, is there a tracking issue for this? It will affect us distro packagers... |
We have previously documented the fact that these will panic on overflow, but I think this behavior is what people actually want/expect.
#[rustc_inherit_overflow_checks]didn't exist when we discussed these for stabilization.r? @alexcrichton
Closes #35807