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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add
Arc::unwrap_or_drop
for safely discardingArc
s without calling the destructor on the inner type. #75911Add
Arc::unwrap_or_drop
for safely discardingArc
s without calling the destructor on the inner type. #75911Changes from 4 commits
a534492
8af2a40
1ceee61
838e5ed
08455a6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the extra-indentation and block are necessary here, you should be able to put everything at the same level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not necessary, I know. I felt like it looked better this way, especially making clear that the comment is referencing the entire following code. I also wrote this when the two examples where not split yet. I have not reevaluated if it looks a bit less confusing to have this block on top level now that the examples are split.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can put a vertical space between the overarching comment and the rest:
The example end with this block so there should be no confusion about it. (I hope)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This second long example feels slightly overkill for this kind of highly specialized method. I would assume that people reaching for this method don't need a motivating real world example anymore. However, now that it's here already, you don't need to remove it. Maybe it helps someone after all ^_^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I know, I called it overkill myself. Nontheless, more examples aren’t going to hurt, probably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments in std usually start uppercase. This also applies to other comments that I won't individually comment on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ll still have to rework the comments a bit anyways, as suggested above. But thanks for the info anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary for now. We can still improve this later, if it seems useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was mostly just curious what the reasoning for the separation in the
drop
implementation is good for. Possibly improved performance by reducing code size in the common case; but someone ought to have none some benchmarking determining that this is worth it somewhere, right? Perhaps I should look at the git blame and track down the original author and PR to find out...I was just thinking that since
unwrap_or_drop
is pretty much also just a destructor forArc
, all the same performance considerations should apply there, too. On the other hand,try_unwrap
does not do anything like this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot: Please write a
// SAFETY:
comment for why this is safe here. It doesn't have to be very long, just explains why the invariants are checked. :) Here is an example fromcore
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The situation is a bit more complex. It is safe because it is doing the same things that the
drop
implementation does, with the difference of doingptr::read
instead ofptr::drop_in_place
, but those two operations are applicable in pretty much exactly the same situations. The reason why every step ofdrop
is safe also includes some really really really long comments indrop
about ordering of atomic operations, hence my general question, as stated in theFIXME
in line 538: “should I copy [...] the comments from drop?”There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I've done before is say something along the lines of "this is safe for the same reason the
drop
impl is safe; see that for more info".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I didn't answer earlier, I didn't get the notification for this comment.
The comments in the
drop
implementation should not change very much, except for improving them so referencing them is probably okay.You can add a line about the differences between
ptr::read
andptr::drop_in_place
and why the former must be used instead of the later below and it should be good.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spawning a thread takes a while, so I would almost expect both
try_unwraps
to never run at the same time. Maybe try spawning both threads but then immediately blocking them to synchronize them. Then after both threads are spawned, in the main thread, you somehow signal to both threads that they may start now. I am not sure how best to do that, though.RwLock
or rendezvous channel maybe?But yeah, in any case, that's a really tricky test to write. Not sure what's best here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested and the test does indeed fail more often than one would think (on my machine) if
try_unwrap(x).ok()
is used (at least on my machine). Which makes me notice... I totally forgot replacing it withunwrap_or_drop
again after testing that these failures can happen!! Damn... this means also means that it didn’t fail once on the CI. But the hope was basically only that ifunwrap_or_drop
changed in the future and became broken (i.e. lost its guarantees of being atomic) then the test would fail for a lot of people (even if not for everyone) and the error would be noticed eventually.