Implement PartialEq for WeakRef

[andy128k]

Does it make sense?

Related to #39

[sdroege]

I think that's OK, yes.

Why did you decide to not also implement Eq, PartialOrd, Ord and Hash though?

[andy128k]

No specific reason. I was just focused on an implementation of WeakMap/WeakSet and needed PartialEq.

I've just added Eq and Hash. Not sure if Ord for pointers makes sense.

[sdroege]

Hm thinking about it a bit more, it's a bit problematic that the pointer value can just change. So if you store this in a HashMap or BTreeMap its position can randomly change. OTOH that's also the case with RefCell (BTreeMap, it has an Ord impl) but that seems like a bug to me.

I think Hash is wrong and only PartialOrd, PartialEq are potentially OK. What do you think?

[andy128k]

I don't use BTreeMap or HashMap but just a Vec<WeakRef<_>>, so I would be happy with sole PartialEq.

[andy128k]

impl Hash is removed

[andy128k]

I think PartialOrd/Ord could be confusing for someone ordering WeakRef<gtk::StringObject>.

[andy128k]

I was looking how weak references are implemented in gobject and they seem pretty expensive (relatively).

I want to have something better than x.upgrade() == Some(y) and x.upgrade() == y.upgrade().

[sdroege]

I think PartialOrd/Ord could be confusing for someone ordering WeakRef<gtk::StringObject>.

PartialOrd / Ord are implemented on StringObject though so that would only be consistent.

I was looking how weak references are implemented in gobject and they seem pretty expensive (relatively).

Yes

---

In summary, I think PartialOrd and PartialEq are fine but the others not.

[sdroege]

In summary, I think PartialOrd and PartialEq are fine but the others not.

The others (that are not fine) include Ord and Eq if that was not clear. AFAIU their requirements are violated by the pointer possibly changing randomly.

[sdroege]

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f68c45adc9f9b1fe064be663807a1b2c FWIW for the problem I mentioned. Funny enough there is even a clippy lint warning about that: https://rust-lang.github.io/rust-clippy/master/index.html#mutable_key_type