Use explicit wrapping_add … #43588
Use explicit wrapping_add … #43588
Conversation
|
r? @sfackler (rust_highfive has picked a reviewer for you, use r? to override) |
src/libstd/sync/barrier.rs
Outdated
| @@ -141,7 +141,7 @@ impl Barrier { | |||
| pub fn wait(&self) -> BarrierWaitResult { | |||
| let mut lock = self.lock.lock().unwrap(); | |||
| let local_gen = lock.generation_id; | |||
| lock.count += 1; | |||
| lock.count.wrapping_add(1); | |||
There was a problem hiding this comment.
You need lock.count = lock.count.wrapping_add(1) (wrapping_add returns the new value instead of modifying in-place).
There was a problem hiding this comment.
How can this ever overflow? Wouldn't that require more than usize::MAX_VALUE threads?
There was a problem hiding this comment.
You are both right. This can not overflow and the syntax was wrong. Thank you
aidanhs
added
the
S-waiting-on-author
|
Hi @dns2utf8. This PR needs fixing - are you still working on it? Just a friendly ping to keep this on your radar. Also, could you provide an example in which an overflow can happen, to make sure it is handled correctly? |
|
Thank you for the ping. The example I would like to prevent is when reusing a barrier the generation could trigger an overflow panic at some point. |
|
@bors r+ |
|
📌 Commit 702750c has been approved by |
|
⌛ Testing commit 702750c with merge 75617d7c06406ef8db44f6f12d4cb8eaabbe4130... |
|
☀️ Test successful - status-appveyor, status-travis |
|
👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward |
Use explicit wrapping_add … … to prevent potential unexpected behavior on debug builds.
|
☀️ Test successful - status-appveyor, status-travis |
|
Cool! Thank you 😄 |
… to prevent potential unexpected behavior on debug builds.