Panic on overflow in Duration::new constructor#33072
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
Panicking on overflow is also done for `+`, and it replaces the currently incorrect overflow behavior of wrapping around, which does not make sense for `Duration`s.
tbu-
force-pushed
the
pr_duration_new_overflow
branch
from
b975c3c to
b25bb53
Compare
|
Instead of panicking, wouldn't it be better to return a |
|
Shouldn't it just use the regular debug assertions for overflow? Those should work fine here? |
|
@bluss I don't think performance is a concern here, the constructor is most likely only called with constant parameters anyway. |
|
@GuillaumeGomez The constructor is a stable function, changing the return type is probably not an option. Also, as above, the constructor is most likely called with constant parameters, so it would mostly be a nuisance to require unwrapping a |
|
This change is probably not compatible with #33033. |
alexcrichton
added
the
T-libs-api
|
Sounds like something that's good to discuss! Currently this seems consistent with the rest of |
so we can declare constants like The assertion in const fn is the same issue (or at least similar to, since it's not a safety concern) as rust-lang/rfcs#1383 cc @pnkfelix |
|
⌛ Testing commit b25bb53 with merge 0144bc5... |
|
💔 Test failed - auto-win-gnu-32-opt |
|
Failure seems unrelated. |
Panic on overflow in `Duration::new` constructor Panicking on overflow is also done for `+`, and it replaces the currently incorrect overflow behavior of wrapping around, which does not make sense for `Duration`s.
Here is an idea: |
you've been beaten to it |
Ah, good to know such a proposal already exists. |
9 participants
Panicking on overflow is also done for
+, and it replaces thecurrently incorrect overflow behavior of wrapping around, which does not
make sense for
Durations.