Comment on Rc abort-guard strategy without naming unrelated fn
#140483
Conversation
`wrapped_add` is used, not `checked_add`
|
rustbot has assigned @workingjubilee. Use |
rustbot
added
S-waiting-on-review
library/alloc/src/rc.rs
Outdated
| @@ -3536,7 +3536,7 @@ impl<T> Default for Weak<T> { | |||
| } | |||
| } | |||
|
|
|||
| // NOTE: We checked_add here to deal with mem::forget safely. In particular | |||
| // NOTE: We wrapping_add here to deal with mem::forget safely. In particular | |||
There was a problem hiding this comment.
This is confusing in a different way: the wrapping_add does not offer the protection anymore, the strong == 0 conditional does.
There was a problem hiding this comment.
at the very least, it should mention clearly what actually implements the guard that handles the mem::forget, if it's going to mention any code.
There was a problem hiding this comment.
I see what you mean. The fact that it's wrapping_add rather than checked_add previously is merely a detail for the sake of code generation, and that's addressed in a more local comment. In that case, I think this comment is more about the higher level issue of a potential soundness hole and isn't enhanced by mentioning the particular implementation details. How about this?
// NOTE: If you mem::forget Rcs (or Weaks), drop is skipped and the ref-count
// is not decremented, meaning the ref-count can overflow, and then you can
// free the allocation while outstanding Rcs (or Weaks) exist, which would be
// unsound. We abort because this is such a degenerate scenario that we don't
// care about what happens -- no real program should ever experience this.
There was a problem hiding this comment.
Yeah, something like that seems good to me.
|
I think this requires more than a one-word change to make something that makes sense. At least in the current mistaken form it's obviously " @rustbot author |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
Update comment per review feedback
|
@rustbot ready |
rustbot
added
S-waiting-on-review
workingjubilee
changed the title
Rc abort-guard strategy without naming unrelated fn
|
Thanks! @bors r+ rollup |
bors
added
S-waiting-on-bors
Rollup of 9 pull requests Successful merges: - rust-lang#134273 (de-stabilize bench attribute) - rust-lang#139534 (Added support for `apxf` target feature) - rust-lang#140419 (Move `in_external_macro` to `SyntaxContext`) - rust-lang#140483 (Comment on `Rc` abort-guard strategy without naming unrelated fn) - rust-lang#140607 (support duplicate entries in the opaque_type_storage) - rust-lang#140656 (collect all Fuchsia bindings into the `fuchsia` module) - rust-lang#140668 (Implement `VecDeque::truncate_front()`) - rust-lang#140709 (rustdoc: remove unportable markdown lint and old parser) - rust-lang#140713 (Structurally resolve in `check_ref_cast` in new solver) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#140483 - baumanj:patch-1, r=workingjubilee Comment on `Rc` abort-guard strategy without naming unrelated fn `wrapped_add` is used, not `checked_add`, so avoid mentioning specific fn calls that may vary slightly based on "whatever produces the best code" and focus on things that will remain constant into the future.
wrapped_addis used, notchecked_add, so avoid mentioning specific fn calls that may vary slightly based on "whatever produces the best code" and focus on things that will remain constant into the future.