Skip to content
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

Tracking issue for std::ptr::eq, Arc::ptr_eq, and Rc::ptr_eq #36497

Closed
SimonSapin opened this issue Sep 15, 2016 · 11 comments
Closed

Tracking issue for std::ptr::eq, Arc::ptr_eq, and Rc::ptr_eq #36497

SimonSapin opened this issue Sep 15, 2016 · 11 comments
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@SimonSapin
Copy link
Contributor

SimonSapin commented Sep 15, 2016

Implemented in #35992 as #[unstable(feature = "ptr_eq")].

std::ptr::eq is rust-lang/rfcs#1155.

@aturon aturon added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Sep 15, 2016
@vadixidav
Copy link
Contributor

vadixidav commented Nov 25, 2016

I needed Rc::ptr_eq to avoid a double borrow in a recursive data structure with Rc<RefCell< T >>. Thanks a lot!

@SimonSapin
Copy link
Contributor Author

@vadixidav It can be implemented outside of std by tweaking it a bit. Copy-pasting this into your code can unblock you until Rc::prt_eq is stabilized:

pub fn rc_ptr_eq<T: ?Sized>(this: &Rc<T>, other: &Rc<T>) -> bool {
    let this_ptr: *const T = &*this;
    let other_ptr: *const T = &*other;
    this_ptr == other_ptr
}

@alexcrichton
Copy link
Member

Seem like good/easy APIs to stabilize!

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Feb 14, 2017

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@federicomenaquintero
Copy link
Contributor

FWIW, I just found a good use for this in librsvg's port to Rust.

Librsvg has a Node structure for the tree of SVG objects. The C code keeps around RsvgNode* values all over the place, including multiple references to the same value.

Sometimes there is code like if (current_node == tree_root) for special-casing... and you see where this is going.

The things I expose to the C code are pointers that come from Box<Rc<Node>> I also expose node_ref() and node_unref() functions so that C can get new references to existing nodes and discard them as needed.

This means that

  RsvgNode *node1 = rsvg_rust_cnode_new (...);  /* returns a Box::into_raw (Box::new (Rc::new (Node))) */
  RsvgNode *node2 = rsvg_node_ref (node1);

  assert (node1 == node2);

is invalid now. What I want is to

  assert (rsvg_node_is_same (node1, node2));

and that can be implemented as

pub type RsvgNode = Rc<Node>;

#[no_mangle]
pub extern fn rsvg_node_is_same (raw_node1: *const RsvgNode, raw_node2: *const RsvgNode) -> bool {
    if raw_node1.is_null () && raw_node2.is_null () {
        true
    } else if !raw_node1.is_null () && !raw_node2.is_null () {
        let node1: &RsvgNode = unsafe { & *raw_node1 };
        let node2: &RsvgNode = unsafe { & *raw_node2 };

        Rc::ptr_eq (node1, node2)
    } else {
        false
    }
}

For now I'm using @SimonSapin's code (it's missing double asterisks in &**this and &**other), but it would indeed be nice to have an Rc::ptr_eq() in stable!

@SimonSapin
Copy link
Contributor Author

For now I'm using @SimonSapin's code

I sometimes find convenient to make it less generic (when it’s only used with one T type):

fn same_node(a: *const Node, b: *const Node) -> bool {
    a == b
}

This looks the same as ==, but since it expects raw pointer they can be coerced implicitly from references. (And, by the way, (*const T)::as_ref() can be more convenient than is_null)

#[no_mangle]
pub extern fn rsvg_node_is_same (raw_node1: *const RsvgNode, raw_node2: *const RsvgNode) -> bool {
    match (raw_node1.as_ref(), raw_node2.as_ref()) {
        (Some(node1), Some(node2)) => same_node(&**node1, &**node2),
        (None, None) => true,
        _ => false
    }
}

(Of course, Rc::eq is nicer still.)

@alexcrichton
Copy link
Member

ping @BurntSushi (checkbox)

@rfcbot
Copy link

rfcbot commented Feb 28, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Feb 28, 2017
@rfcbot
Copy link

rfcbot commented Mar 10, 2017

The final comment period is now complete.

frewsxcv pushed a commit to frewsxcv/rust that referenced this issue Mar 17, 2017
alexcrichton pushed a commit to alexcrichton/rust that referenced this issue Mar 17, 2017
@bors bors closed this as completed in 10510ae Mar 19, 2017
@matklad
Copy link
Member

matklad commented Dec 31, 2020

For future GitHub travelers, investigating clippy::vtable_address_comparisons:

Looks like the methods were stabilized with the ?Sized bound, while the current impl doesn't quite handle them. Specifically, ptr_eq can spuriously return false for identical trait objects. This is being tracked in #69757.

@SimonSapin
Copy link
Contributor Author

This is not unique to these ptr_eq methods. Wide pointers to trait objects have been comparable with the == operator forever, with the same spurious results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants