Trap weighted index overflow#1353
Conversation
39b0c17 to
4deeff2
Compare
|
So We want Alternative: use So I think it comes down to:
I opted for the second choice here: /// Bounds on a weight
///
/// See usage in [`WeightedIndex`].
pub trait Weight: Clone {
/// Representation of 0
const ZERO: Self;
/// Checked addition
///
/// - `Result::Ok`: On success, `v` is added to `self`
/// - `Result::Err`: Returns an error when `Self` cannot represent the
/// result of `self + v` (i.e. overflow). The value of `self` should be
/// discarded.
fn checked_add_assign(&mut self, v: &Self) -> Result<(), ()>;
} |
|
What this omits: the impls of Also: the signature of |
|
I really like the simplified trait bounds! My only concern is that |
dhardy
left a comment
There was a problem hiding this comment.
Yes, the bounds now look much nicer!
| /// Checked addition | ||
| /// | ||
| /// - `Result::Ok`: On success, `v` is added to `self` | ||
| /// - `Result::Err`: Returns an error when `Self` cannot represent the | ||
| /// result of `self + v` (i.e. overflow). The value of `self` should be | ||
| /// discarded. | ||
| fn checked_add_assign(&mut self, v: &Self) -> Result<(), ()>; |
There was a problem hiding this comment.
The only thing that concerns me slightly is that this is a fairly complex signature that, as you say, we can't easily change later.
Possibly the standard checked_add signature would be better, but this one is closer to what we actually need (considering we don't have a Copy bound).
Closes #1309. This approach is likely sub-optimal but without significant overhead (not benchmarked), and only to a distribution constructor.