Add pub fn ptr_eq(this: &Self, other: &Self) -> bool to Rc and Arc

[SimonSapin]

Servo and Kuchiki have had helper functions doing this for some time.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[aturon]

cc @rust-lang/libs

I feel like there's some history here -- e.g. it used to exist -- but I'm not sure when/why this was removed offhand.

In general I think this is a nice convenience to provide.

[frewsxcv]

Travis failures.

[alexcrichton]

I guess I don't see a reason to not have these in the standard library, but conversely I don't see a particular reason to have these in the standard library either. I've written them from time to time as well, but I never felt like there was a footgun or anything, the implementation was pretty clear.

[Stebalien]

Here's a more general solution to the same problem (might make a useful crate): https://gist.github.com/61d5a030296ad3d5b0c3ea71c088c836

[SimonSapin]

(Side question: I’ve pushed what I think fixes the doctests, but make check-stage1 is still running. Is there a make invocation that runs just the doctests for a given crate, like check-stage1-alloc runs just the unit tests?)

Yes, it is possible and not particularly difficult to implement outside the standard library, but it seems to be a pretty basic operation when dealing with reference-counted or garbage-collected things. Both Python and Java have operators (is and ==, respectively) built into the language for this.

And Rust’s std already has plenty of precedent for including small convenience methods that could be implemented outside.

The only (small) footgun that I know of is being tempted to make a single generic function that works for both Rc<T> and Arc<T>. I’ve written that function, and there is such a thing as too generic: it becomes easy to accidentally compare the addresses of Rc<T> references instead of the referencedRcInnerorT`. @Stebalien’s code avoids this problem by introducing a trait with a recursive method, but that requires (today still unstable) trait specialization.

[aturon]

Yes, it is possible and not particularly difficult to implement outside the standard library, but it seems to be a pretty basic operation when dealing with reference-counted or garbage-collected things.

Agreed. I think we should ship it.

[brson]

Functionality seems fine to me. Is ptr_eq the right name? I don't see that name anywhere in std today (nor ref_eq). Is there really no precedent for this?

[SimonSapin]

The ptr abbreviation has precedent in the std::ptr module’s name. The point of this functionality is to use (almost) <*cont T as PartialEq>::eq (as opposed to <T as PartialEq>::eq).

[alexcrichton]

Discussed at libs triage the conclusion was that these seem good to add. @Kimundi also pointed out that this is very similar to rust-lang/rfcs#1155 and we were also thinking it'd be good to add:

fn std::mem::ptr_eq<T: ?Sized>(a: &T, b: &T) -> bool;

@SimonSapin would you be interested in adding those methods as well? Also could you update the tracking issue reference here as well with a fresh issue?

[SimonSapin]

Sure, I can add this. Although in the RFC the arguments are *const T. (They can be used with &T thought coercion, the docs would give examples of this.) Do you want to change them to &T? I think *const T fits better in the std::ptr module and is maybe easier to document.

Should these be three "features" and tracking issues, or just one?

[sfackler]

I kind of like std::ptr::eq(a, b) now that I think about it. Should be fine ergonomically since we coerce.

[SimonSapin]

std::ptr::eq is useful to compare &T references (coerced to *const T).

Just to be clear however, I don’t think it works as a replacement for Arc::ptr_eq or Rc::ptr_eq. For example, let a: Arc<T> = …; let b: Arc<T> = …; std::ptr::eq(&a, &b) will end up comparing *cont Arc<T> pointers to the Arc pointers on the stack, not comparing *const ArcInner<T> like Arc::ptr_eq would.

I’ll update the PR to add all three.

[sfackler]

Oh yeah, we absolutely want all three.

[SimonSapin]

Filed #36497 and updated the PR.

[alexcrichton]

@bors: r+

[bors]

📌 Commit 5ce9fee has been approved by alexcrichton

[bors]

⌛ Testing commit 5ce9fee with merge d1acabe...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild
Approved by: alexcrichton
Pushing d1acabe to master...

[JoeyAcc]

Could this reliably be used to perform an equality check for functions using pointers?

For example:

fn ein() { }

fn zwei() { }

fn main() {
    println!("ein == zwei: {}", ein as *mut () == zwei as *mut ());  // false
    println!("ein == zwei: {}", ein as *mut () == ein as *mut ());   // true
}

The above works on stable, so what I'm really wondering about is how reliably this can be used.