Add new_cyclic_in for Rc and Arc #129674
Merged
Add new_cyclic_in for Rc and Arc #129674
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @workingjubilee (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
S-waiting-on-review
|
@rustbot author |
rustbot
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
|
@rustbot review |
rustbot
added
S-waiting-on-review
You can avoid |
|
Maybe change |
Thank you! Fixed this in the latest commit. Destructuring the
Is it then possible to turn the returned Edit: It seems like just calling |
|
r? libs-api |
rustbot
added
the
T-libs-api
dtolnay
approved these changes
Sep 14, 2024
There was a problem hiding this comment.
Thank you!
If you care about having these commits credited to your GitHub account in https://github.com/rust-lang/rust/graphs/contributors, you'll need to add your school email to https://github.com/settings/emails.
|
@bors r+ |
|
@bors r- |
bors
added
S-waiting-on-author
dtolnay
requested changes
Sep 14, 2024
Co-authored-by: David Tolnay <dtolnay@gmail.com>
rustbot
added
S-waiting-on-review
dtolnay
approved these changes
Sep 17, 2024
|
@bors r+ |
bors
added
S-waiting-on-bors
fee1-dead
added a commit
to fee1-dead-contrib/rust
that referenced
this pull request
Sep 17, 2024
… r=dtolnay Add new_cyclic_in for Rc and Arc Currently, new_cyclic_in does not exist for Rc and Arc. This is an oversight according to rust-lang/wg-allocators#132. This PR adds new_cyclic_in for Rc and Arc. The implementation is almost the exact same as new_cyclic with some small differences to make it allocator-specific. new_cyclic's implementation has been replaced with a call to `new_cyclic_in(data_fn, Global)`. Remaining questions: * ~~Is requiring Allocator to be Clone OK? According to rust-lang/wg-allocators#88, Allocators should be cheap to clone. I'm just hesitant to add unnecessary constraints, though I don't see an obvious workaround for this function since many called functions in new_cyclic_in expect an owned Allocator. I see Allocator.by_ref() as an option, but that doesn't work on when creating Weak { ptr: init_ptr, alloc: alloc.clone() }, because the type of Weak then becomes Weak<T, &A> which is incompatible.~~ Fixed, thank you `@zakarumych!` This PR no longer requires the allocator to be Clone. * Currently, new_cyclic_in's documentation is almost entirely copy-pasted from new_cyclic, with minor tweaks to make it more accurate (e.g. Rc<T> -> Rc<T, A>). The example section is removed to mitigate redundancy and instead redirects to cyclic_in. Is this appropriate? * ~~The comments in new_cyclic_in (and much of the implementation) are also copy-pasted from new_cyclic. Would it be better to make a helper method new_cyclic_in_internal that both functions call, with either Global or the custom allocator? I'm not sure if that's even possible, since the internal method would have to return Arc<T, Global> and I don't know if it's possible to "downcast" that to an Arc<T>. Maybe transmute would work here?~~ Done, thanks `@zakarumych` * Arc::new_cyclic is #[inline], but Rc::new_cyclic is not. Which is preferred? * nit: does it matter where in the impl block new_cyclic_in is defined?
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 17, 2024
…fee1-dead Rollup of 8 pull requests Successful merges: - rust-lang#128961 (Fix rust-lang#128930: Print documentation of CLI options missing their arg) - rust-lang#129073 (Relate receiver invariantly in method probe for `Mode::Path`) - rust-lang#129674 (Add new_cyclic_in for Rc and Arc) - rust-lang#130201 (Encode `coroutine_by_move_body_def_id` in crate metadata) - rust-lang#130275 (Don't call `extern_crate` when local crate name is the same as a dependency and we have a trait error) - rust-lang#130440 (Don't ICE in `opaque_hidden_inferred_bound` lint for RPITIT in trait with no default method body) - rust-lang#130454 (tests: allow trunc/select instructions to be missing) - rust-lang#130458 (`rustc_codegen_ssa` cleanups) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 17, 2024
…=dtolnay Add new_cyclic_in for Rc and Arc Currently, new_cyclic_in does not exist for Rc and Arc. This is an oversight according to rust-lang/wg-allocators#132. This PR adds new_cyclic_in for Rc and Arc. The implementation is almost the exact same as new_cyclic with some small differences to make it allocator-specific. new_cyclic's implementation has been replaced with a call to `new_cyclic_in(data_fn, Global)`. Remaining questions: * ~~Is requiring Allocator to be Clone OK? According to rust-lang/wg-allocators#88, Allocators should be cheap to clone. I'm just hesitant to add unnecessary constraints, though I don't see an obvious workaround for this function since many called functions in new_cyclic_in expect an owned Allocator. I see Allocator.by_ref() as an option, but that doesn't work on when creating Weak { ptr: init_ptr, alloc: alloc.clone() }, because the type of Weak then becomes Weak<T, &A> which is incompatible.~~ Fixed, thank you `@zakarumych!` This PR no longer requires the allocator to be Clone. * Currently, new_cyclic_in's documentation is almost entirely copy-pasted from new_cyclic, with minor tweaks to make it more accurate (e.g. Rc<T> -> Rc<T, A>). The example section is removed to mitigate redundancy and instead redirects to cyclic_in. Is this appropriate? * ~~The comments in new_cyclic_in (and much of the implementation) are also copy-pasted from new_cyclic. Would it be better to make a helper method new_cyclic_in_internal that both functions call, with either Global or the custom allocator? I'm not sure if that's even possible, since the internal method would have to return Arc<T, Global> and I don't know if it's possible to "downcast" that to an Arc<T>. Maybe transmute would work here?~~ Done, thanks `@zakarumych` * Arc::new_cyclic is #[inline], but Rc::new_cyclic is not. Which is preferred? * nit: does it matter where in the impl block new_cyclic_in is defined?
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
"Empty/Tombstone value shouldn't be inserted into map!" — I've seen the same LLVM crash previously in #123436 (comment), and it succeeded after retry.... @bors retry |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 17, 2024
…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#129477 (Fix fluent diagnostics) - rust-lang#129674 (Add new_cyclic_in for Rc and Arc) - rust-lang#130452 (Update Trusty target maintainers) - rust-lang#130467 (Miri subtree update) - rust-lang#130477 (Revert rust-lang#129749 to fix segfault in LLVM) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 17, 2024
Rollup merge of rust-lang#129674 - matthewpipie:rc-arc-new-cyclic-in, r=dtolnay Add new_cyclic_in for Rc and Arc Currently, new_cyclic_in does not exist for Rc and Arc. This is an oversight according to rust-lang/wg-allocators#132. This PR adds new_cyclic_in for Rc and Arc. The implementation is almost the exact same as new_cyclic with some small differences to make it allocator-specific. new_cyclic's implementation has been replaced with a call to `new_cyclic_in(data_fn, Global)`. Remaining questions: * ~~Is requiring Allocator to be Clone OK? According to rust-lang/wg-allocators#88, Allocators should be cheap to clone. I'm just hesitant to add unnecessary constraints, though I don't see an obvious workaround for this function since many called functions in new_cyclic_in expect an owned Allocator. I see Allocator.by_ref() as an option, but that doesn't work on when creating Weak { ptr: init_ptr, alloc: alloc.clone() }, because the type of Weak then becomes Weak<T, &A> which is incompatible.~~ Fixed, thank you `@zakarumych!` This PR no longer requires the allocator to be Clone. * Currently, new_cyclic_in's documentation is almost entirely copy-pasted from new_cyclic, with minor tweaks to make it more accurate (e.g. Rc<T> -> Rc<T, A>). The example section is removed to mitigate redundancy and instead redirects to cyclic_in. Is this appropriate? * ~~The comments in new_cyclic_in (and much of the implementation) are also copy-pasted from new_cyclic. Would it be better to make a helper method new_cyclic_in_internal that both functions call, with either Global or the custom allocator? I'm not sure if that's even possible, since the internal method would have to return Arc<T, Global> and I don't know if it's possible to "downcast" that to an Arc<T>. Maybe transmute would work here?~~ Done, thanks `@zakarumych` * Arc::new_cyclic is #[inline], but Rc::new_cyclic is not. Which is preferred? * nit: does it matter where in the impl block new_cyclic_in is defined?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, new_cyclic_in does not exist for Rc and Arc. This is an oversight according to rust-lang/wg-allocators#132.
This PR adds new_cyclic_in for Rc and Arc. The implementation is almost the exact same as new_cyclic with some small differences to make it allocator-specific. new_cyclic's implementation has been replaced with a call to
new_cyclic_in(data_fn, Global).Remaining questions:
Is requiring Allocator to be Clone OK? According to Should most allocators be Clone? wg-allocators#88, Allocators should be cheap to clone. I'm just hesitant to add unnecessary constraints, though I don't see an obvious workaround for this function since many called functions in new_cyclic_in expect an owned Allocator. I see Allocator.by_ref() as an option, but that doesn't work on when creating Weak { ptr: init_ptr, alloc: alloc.clone() }, because the type of Weak then becomes Weak<T, &A> which is incompatible.Fixed, thank you @zakarumych! This PR no longer requires the allocator to be Clone.The comments in new_cyclic_in (and much of the implementation) are also copy-pasted from new_cyclic. Would it be better to make a helper method new_cyclic_in_internal that both functions call, with either Global or the custom allocator? I'm not sure if that's even possible, since the internal method would have to return Arc<T, Global> and I don't know if it's possible to "downcast" that to an Arc. Maybe transmute would work here?Done, thanks @zakarumych