Small refactoring to make this code more clear #30593

steveklabnik · 2015-12-28T20:29:56Z

This hairy conditional doesn't need to be so. It does need to be a
thin pointer, otherwise, it will fail to compile, so let's pull that out
into a temporary for future readers of the source.

/cc @nrc @SimonSapin @gankro @durka , who brought this up on IRC

rust-highfive · 2015-12-28T20:30:07Z

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

brson · 2015-12-28T20:35:08Z

@bors r+ rollup

bors · 2015-12-28T20:35:09Z

📌 Commit a1f3781 has been approved by brson

steveklabnik · 2015-12-28T21:06:11Z

@bors: r-

steveklabnik · 2015-12-28T21:06:22Z

This failed with a segfault on travis. I thought that my local build worked, double checking now.

steveklabnik · 2015-12-28T21:29:05Z

Wow, this does segfault locally, and also with this small second change that @aturon pointed out.

Maybe there is some kind of magic in the original mess there...

steveklabnik · 2015-12-28T21:42:36Z

re-reading again, I noticed that there was one level of indirection at one arm, and another at the other arm. So I did this third commit, and it also segfaults.

At this point, I'm just wondering what magic here is causing this...

nagisa · 2015-12-28T21:54:37Z

src/liballoc/arc.rs

-        if ptr as *mut u8 as usize == 0 || ptr as *mut u8 as usize == mem::POST_DROP_USIZE {
+        let thin = ptr as *const ();
+
+        if !thin.is_null() && thin as usize != mem::POST_DROP_USIZE {


Why did you invert the condition?

It should either be if thin.is_null() || thin as usizee == mem::POST_DROP_USIZE or if !(!thin.is_null() && thin as usize != mem::POST_DROP_USIZE). What you wrote is not equivalent to the old code.

... because even simple code is hard. that'd do it, i'd bet.

Would it make sense to abstract the condition to a function, so that it can be given a name and re-used? The code itself is trivial, but it's still easy to get it wrong. if already_dropped(ptr) { return; } (or any other better name) would be easy to read and convey the information about what is going on in both places (currently the check at https://github.com/steveklabnik/rust/blob/small_rc_refactoring/src/liballoc/arc.rs#L715 refers to the comment of the other code fragment, which finally explains what is going on).

steveklabnik · 2015-12-28T23:27:39Z

This no longer segfaults, but it still fails several Arc tests. I have to run, I will fix the tests in the morning. I'm sure I just did something else dumb, time to bust out some truth tables.

steveklabnik · 2015-12-29T19:44:08Z

Okay! Finally got it to pass. I was just being silly. I've squashed and rebased everything.

Gankra · 2015-12-29T20:33:43Z

Is the fact that the two files are divergent intentional?

steveklabnik · 2015-12-29T20:50:36Z

They were before I got here.

Gankra · 2015-12-30T00:29:44Z

Is there a reason to preserve the divergence?

Gankra · 2015-12-30T00:30:30Z

src/liballoc/rc.rs

-               ptr as *const () as usize != mem::POST_DROP_USIZE {
+            let thin = ptr as *const ();
+
+            if !(thin as *const *const ()).is_null() && thin as usize != mem::POST_DROP_USIZE {


In particular this cast to *const *const () seems pointless?

I had re-added it when @aturon pointed out that I was missing the second level. I thought it was pointless, but I thought it might have been causing the segfaults.

On Dec 29, 2015, 19:30 -0500, Alexis Beingessnernotifications@github.com, wrote:

Insrc/liballoc/rc.rs(#30593 (comment)):

@@ -782,8 +782,9 @@ impl<T: ?Sized>Drop for Weak{>fn drop(&mut self) {>unsafe {>let ptr = _self.ptr;>- if !((&ptr as *const _ as *const *const ())).is_null()&&>- ptr as *const () as usize != mem::POST_DROP_USIZE {>+ let thin = ptr as *const ();>+>+ if !(thin as *const *const ()).is_null()&&thin as usize != mem::POST_DROP_USIZE {

In particular this cast seems pointless?

—
Reply to this email directly orview it on GitHub(https://github.com/rust-lang/rust/pull/30593/files#r48580830).

If it's causing the segfaults, this should be filed as a bug.

There's a bug open for it generally, I think #30577

but I'm not sure yet, as I didn't really dig into the why.

It does work without this cast. PR updated.

steveklabnik · 2015-12-30T14:26:52Z

Is there a reason to preserve the divergence?

No, other than I'm not the person to write that patch, just trying to do a small cleanup here.

ranma42 · 2015-12-30T15:27:47Z

Is there any reason why the comparisons at https://github.com/steveklabnik/rust/blob/cd03c83330df5aa9a030b632e80e6675e26615ec/src/liballoc/arc.rs#L715 and https://github.com/steveklabnik/rust/blob/cd03c83330df5aa9a030b632e80e6675e26615ec/src/liballoc/rc.rs#L452 are not updated as well? Should that be in another PR?

steveklabnik · 2015-12-30T15:31:39Z

Only because I didn't notice them! I'll do that right now.

steveklabnik · 2015-12-30T15:41:38Z

@gankro oh, did you mean the divergence of the ! || == != stuff? Yes, there's no reason to preserve that divergence. I thought you meant other things in the file. I'm testing a thing right now with them all having completely identical conditionals, now that I know that the extra cast isn't needed.

Gankra · 2015-12-30T15:43:48Z

The & vs | divergence I'm kinda fine with preserving. iirc it "makes sense" for each drop impl.

steveklabnik · 2015-12-30T15:56:27Z

Okay, so this is very strange:

diff --git a/src/liballoc/rc.rs b/src/liballoc/rc.rs
index 9109e9d..8318607 100644
--- a/src/liballoc/rc.rs
+++ b/src/liballoc/rc.rs
@@ -784,7 +784,7 @@ impl<T: ?Sized> Drop for Weak<T> {
             let ptr = *self._ptr;
             let thin = ptr as *const ();

-            if !thin.is_null() && thin as usize != mem::POST_DROP_USIZE {
+            if thin.is_null() || thin as usize == mem::POST_DROP_USIZE {
                 self.dec_weak();
                 // the weak count starts at 1, and will only go to zero if all
                 // the strong pointers have disappeared.

This diff causes a segfault.

Gankra · 2015-12-30T15:57:54Z

That's because it's the exact opposite of the check. You now dec_weak only if the pointer is null or dropped.

You left out a negation in your demorgans.

steveklabnik · 2015-12-30T16:02:18Z

Oh duh. It still probably shouldn't segfault, but yes, I am just being dumb. :(

Gankra · 2015-12-30T16:04:29Z

Nah derefing null/dangling ptrs is pretty much the definition of "should segfault"

steveklabnik · 2015-12-30T16:05:32Z

I guess this is unsafe.

I should like, not write code until 2pm or something.

steveklabnik · 2015-12-30T16:25:16Z

I have fixed my errors and also handled the two other places as well.

@pnkfelix

This hairy conditional doesn't need to be so. It _does_ need to be a thin pointer, otherwise, it will fail to compile, so let's pull that out into a temporary for future readers of the source. Also, after a discussion with @pnkfelix and @gankro, we don't need these null checks anymore, as zero-on-drop has been gone for a while now.

steveklabnik · 2015-12-30T17:08:11Z

@pnkfelix and @gankro noticed that we don't need the null checks anymore, either 👍

Gankra · 2015-12-30T17:20:32Z

@bors r+

WE DID IT

bors · 2015-12-30T17:20:32Z

📌 Commit 2cff12e has been approved by Gankro

bors · 2015-12-31T11:42:17Z

⌛ Testing commit 2cff12e with merge 8aee582...

@nrc

This hairy conditional doesn't need to be so. It _does_ need to be a thin pointer, otherwise, it will fail to compile, so let's pull that out into a temporary for future readers of the source. /cc @nrc @SimonSapin @gankro @durka , who brought this up on IRC

bors · 2015-12-31T13:34:52Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

rust-highfive assigned brson Dec 28, 2015

nagisa reviewed Dec 28, 2015
View reviewed changes

steveklabnik force-pushed the small_rc_refactoring branch from e1caee1 to b5b2602 Compare December 29, 2015 19:43

Gankra reviewed Dec 30, 2015
View reviewed changes

steveklabnik force-pushed the small_rc_refactoring branch from b5b2602 to cd03c83 Compare December 30, 2015 14:33

steveklabnik force-pushed the small_rc_refactoring branch from 5d48ad6 to 88a389c Compare December 30, 2015 16:25

steveklabnik force-pushed the small_rc_refactoring branch from 88a389c to 7426f85 Compare December 30, 2015 16:29

steveklabnik force-pushed the small_rc_refactoring branch from 7426f85 to 0182a77 Compare December 30, 2015 16:29

steveklabnik force-pushed the small_rc_refactoring branch from 0182a77 to 2cff12e Compare December 30, 2015 17:06

bors merged commit 2cff12e into rust-lang:master Dec 31, 2015

steveklabnik deleted the small_rc_refactoring branch June 19, 2016 20:30

Small refactoring to make this code more clear #30593

Small refactoring to make this code more clear #30593

Conversation

steveklabnik commented Dec 28, 2015

rust-highfive commented Dec 28, 2015

brson commented Dec 28, 2015

bors commented Dec 28, 2015

steveklabnik commented Dec 28, 2015

steveklabnik commented Dec 28, 2015

steveklabnik commented Dec 28, 2015

steveklabnik commented Dec 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklabnik commented Dec 28, 2015

steveklabnik commented Dec 29, 2015

Gankra commented Dec 29, 2015

steveklabnik commented Dec 29, 2015

Gankra commented Dec 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklabnik commented Dec 30, 2015

ranma42 commented Dec 30, 2015

steveklabnik commented Dec 30, 2015

steveklabnik commented Dec 30, 2015

Gankra commented Dec 30, 2015

steveklabnik commented Dec 30, 2015

Gankra commented Dec 30, 2015

steveklabnik commented Dec 30, 2015

Gankra commented Dec 30, 2015

steveklabnik commented Dec 30, 2015

steveklabnik commented Dec 30, 2015

steveklabnik commented Dec 30, 2015

Gankra commented Dec 30, 2015

bors commented Dec 30, 2015

bors commented Dec 31, 2015

bors commented Dec 31, 2015