Define stable ordering for WeakPointerTo for use as map/set key

Currently, the WeakPointerTo comparison operators compare the raw pointers, but this is not useful as it may cause a false equality if one weak pointer in the comparison is expired and points to memory that has since been reused.

Instead, we can define a comparison based on the control block pointer, which exists since the new weak pointer implementation in 0bb81a43c9e4fffb37cc2234c1b0fbae42020ceb.  This is implemented in the owner_before method, matching C++11 std::weak_ptr semantics.

I would now recommend deprecating most comparison operators of WeakPointerTo or redefining them to make more sense, ie. comparing equal if they (once) referred to the same object and not if they simply point to the same memory address.  This has not yet been done, though code that uses the comparison operators has been fixed in this commit.

Overloads of std::owner_less have been provided for creating a map or set with Weak(Const)PointerTo keys.

dtool/src/dtoolbase/dtoolbase_cc.h

@@ -111,6 +111,8 @@ namespace std {
   template<class T> typename remove_reference<T>::type &&move(T &&t) {
     return static_cast<typename remove_reference<T>::type&&>(t);
   }
+
+  template<class T> struct owner_less;
 };
 #endif
 

panda/src/chan/partBundle.h

@@ -172,7 +172,7 @@ private:
   typedef pvector<PartBundleNode *> Nodes;
   Nodes _nodes;
 
-  typedef pmap<WCPT(TransformState), WPT(PartBundle) > AppliedTransforms;
+  typedef pmap<WCPT(TransformState), WPT(PartBundle), std::owner_less<WCPT(TransformState)> > AppliedTransforms;
   AppliedTransforms _applied_transforms;
 
   double _update_delay;

panda/src/char/characterJointEffect.I

@@ -35,5 +35,9 @@ get_character() const {
  */
 INLINE bool CharacterJointEffect::
 matches_character(Character *character) const {
-  return _character == character;
+  // This works because while the Character is destructing, the ref count will
+  // be 0 but was_deleted() will still return false.  We cannot construct a
+  // PointerTo to the character (via lock() or otherwise) when the reference
+  // count is 0 since that will cause double deletion.
+  return _character.get_orig() == character && !_character.was_deleted();
 }

panda/src/dxgsg9/dxGeomMunger9.cxx

@@ -300,8 +300,11 @@ compare_to_impl(const GeomMunger *other) const {
   if (_filtered_texture != om->_filtered_texture) {
     return _filtered_texture < om->_filtered_texture ? -1 : 1;
   }
-  if (_tex_gen != om->_tex_gen) {
-    return _tex_gen < om->_tex_gen ? -1 : 1;
+  if (_tex_gen.owner_before(om->_tex_gen)) {
+    return -1;
+  }
+  if (om->_tex_gen.owner_before(_tex_gen)) {
+    return 1;
   }
 
   return StandardMunger::compare_to_impl(other);
@@ -321,8 +324,11 @@ geom_compare_to_impl(const GeomMunger *other) const {
   if (_filtered_texture != om->_filtered_texture) {
     return _filtered_texture < om->_filtered_texture ? -1 : 1;
   }
-  if (_tex_gen != om->_tex_gen) {
-    return _tex_gen < om->_tex_gen ? -1 : 1;
+  if (_tex_gen.owner_before(om->_tex_gen)) {
+    return -1;
+  }
+  if (om->_tex_gen.owner_before(_tex_gen)) {
+    return 1;
   }
 
   return StandardMunger::geom_compare_to_impl(other);

panda/src/express/pointerTo.h

@@ -215,6 +215,26 @@ void swap(ConstPointerTo<T> &one, ConstPointerTo<T> &two) noexcept {
 }
 
 
+// Define owner_less specializations, for completeness' sake.
+namespace std {
+  template<class T>
+  struct owner_less<PointerTo<T> > {
+    bool operator () (const PointerTo<T> &lhs,
+                      const PointerTo<T> &rhs) const noexcept {
+      return lhs < rhs;
+    }
+  };
+
+  template<class T>
+  struct owner_less<ConstPointerTo<T> > {
+    bool operator () (const ConstPointerTo<T> &lhs,
+                      const ConstPointerTo<T> &rhs) const noexcept {
+      return lhs < rhs;
+    }
+  };
+}
+
+
 // Finally, we'll define a couple of handy abbreviations to save on all that
 // wasted typing time.
 

panda/src/express/pointerToBase.h

@@ -53,6 +53,7 @@ protected:
 
   // This is needed to be able to access the privates of other instantiations.
   template<typename Y> friend class PointerToBase;
+  template<typename Y> friend class WeakPointerToBase;
 
 PUBLISHED:
   ALWAYS_INLINE void clear();

panda/src/express/weakPointerTo.h

@@ -152,6 +152,25 @@ PUBLISHED:
   INLINE void clear() { WeakPointerToBase<T>::clear(); }
 };
 
+// Provide specializations of std::owner_less, for using a WPT as a map key.
+namespace std {
+  template<class T>
+  struct owner_less<WeakPointerTo<T> > {
+    bool operator () (const WeakPointerTo<T> &lhs,
+                      const WeakPointerTo<T> &rhs) const noexcept {
+      return lhs.owner_before(rhs);
+    }
+  };
+
+  template<class T>
+  struct owner_less<WeakConstPointerTo<T> > {
+    bool operator () (const WeakConstPointerTo<T> &lhs,
+                      const WeakConstPointerTo<T> &rhs) const noexcept {
+      return lhs.owner_before(rhs);
+    }
+  };
+}
+
 #define WPT(type) WeakPointerTo< type >
 #define WCPT(type) WeakConstPointerTo< type >
 

panda/src/express/weakPointerToBase.I

@@ -12,7 +12,8 @@
  */
 
 /**
- *
+ * Constructs a weak pointer from a plain pointer (or nullptr).  It is the
+ * caller's responsibility to ensure that it points to a valid object.
  */
 template<class T>
 INLINE WeakPointerToBase<T>::
@@ -27,7 +28,7 @@ WeakPointerToBase(To *ptr) {
 }
 
 /**
- *
+ * Constructs a weak pointer from a reference-counting pointer.
  */
 template<class T>
 INLINE WeakPointerToBase<T>::
@@ -42,24 +43,25 @@ WeakPointerToBase(const PointerToBase<T> &copy) {
 }
 
 /**
- *
+ * Copies a weak pointer.  This is always safe, even for expired pointers.
  */
 template<class T>
 INLINE WeakPointerToBase<T>::
 WeakPointerToBase(const WeakPointerToBase<T> &copy) {
   _void_ptr = copy._void_ptr;
 
-  // Don't bother increasing the weak reference count if the object was
-  // already deleted.
+  // While it is tempting to stop maintaining the control block pointer after
+  // the object has been deleted, we still need it in order to define a
+  // consistent ordering in owner_before.
   WeakReferenceList *weak_ref = copy._weak_ref;
-  if (weak_ref != nullptr && !weak_ref->was_deleted()) {
+  if (weak_ref != nullptr/* && !weak_ref->was_deleted()*/) {
     _weak_ref = copy._weak_ref;
     _weak_ref->ref();
   }
 }
 
 /**
- *
+ * Moves a weak pointer.  This is always safe, even for expired pointers.
  */
 template<class T>
 INLINE WeakPointerToBase<T>::
@@ -71,7 +73,7 @@ WeakPointerToBase(WeakPointerToBase<T> &&from) noexcept {
 }
 
 /**
- *
+ * Moves a weak pointer from a cast-convertible weak pointer type.
  */
 template<class T>
 template<class Y>
@@ -148,10 +150,11 @@ reassign(const WeakPointerToBase<To> &copy) {
     WeakReferenceList *old_ref = (WeakReferenceList *)_weak_ref;
     _void_ptr = new_ptr;
 
-    // Don't bother increasing the weak reference count if the object was
-    // already deleted.
+    // While it is tempting to stop maintaining the control block pointer
+    // after the object has been deleted, we still need it in order to define
+    // a consistent ordering in owner_before.
     WeakReferenceList *weak_ref = copy._weak_ref;
-    if (weak_ref != nullptr && !weak_ref->was_deleted()) {
+    if (weak_ref != nullptr/* && !weak_ref->was_deleted()*/) {
       weak_ref->ref();
       _weak_ref = weak_ref;
     } else {
@@ -371,7 +374,11 @@ operator >= (std::nullptr_t) const {
 }
 
 /**
- *
+ * Returns true if both pointers have the same raw pointer value.  For this to
+ * be meaningful, neither pointer may have expired, since if one has expired
+ * while the other was allocated at the expired pointer's memory address, this
+ * comparison will be true even though they didn't refer to the same object.
+ * @see owner_before
  */
 template<class T>
 INLINE bool WeakPointerToBase<T>::
@@ -380,7 +387,7 @@ operator == (const WeakPointerToBase<To> &other) const {
 }
 
 /**
- *
+ * @see operator ==
  */
 template<class T>
 INLINE bool WeakPointerToBase<T>::
@@ -389,7 +396,8 @@ operator != (const WeakPointerToBase<To> &other) const {
 }
 
 /**
- *
+ * Defines an ordering between WeakPointerTo based on their raw pointer value.
+ * @deprecated Do not use this.  Use owner_before or std::owner_less instead.
  */
 template<class T>
 INLINE bool WeakPointerToBase<T>::
@@ -398,7 +406,8 @@ operator > (const WeakPointerToBase<To> &other) const {
 }
 
 /**
- *
+ * Defines an ordering between WeakPointerTo based on their raw pointer value.
+ * @deprecated Do not use this.  Use owner_before or std::owner_less instead.
  */
 template<class T>
 INLINE bool WeakPointerToBase<T>::
@@ -407,7 +416,8 @@ operator <= (const WeakPointerToBase<To> &other) const {
 }
 
 /**
- *
+ * Defines an ordering between WeakPointerTo based on their raw pointer value.
+ * @deprecated Do not use this.  Use owner_before or std::owner_less instead.
  */
 template<class T>
 INLINE bool WeakPointerToBase<T>::
@@ -416,7 +426,7 @@ operator >= (const WeakPointerToBase<To> &other) const {
 }
 
 /**
- *
+ * Returns true if both pointers point to the same object.
  */
 template<class T>
 INLINE bool WeakPointerToBase<T>::
@@ -425,7 +435,7 @@ operator == (const PointerToBase<To> &other) const {
 }
 
 /**
- *
+ * Returns false if both pointers point to the same object.
  */
 template<class T>
 INLINE bool WeakPointerToBase<T>::
@@ -479,7 +489,8 @@ operator < (std::nullptr_t) const {
 }
 
 /**
- *
+ * Defines an ordering between WeakPointerTo based on their raw pointer value.
+ * @deprecated Do not use this.  Use owner_before or std::owner_less instead.
  */
 template<class T>
 INLINE bool WeakPointerToBase<T>::
@@ -498,6 +509,35 @@ operator < (const PointerToBase<To> &other) const {
 
 #endif  // CPPPARSER
 
+/**
+ * Defines an ordering that is guaranteed to remain consistent even after the
+ * weak pointers have expired.  This may result in two pointers with the same
+ * get_orig() value comparing unequal if one of them is a new object that was
+ * allocated at the same memory address as the older, expired pointer.
+ */
+template<class T>
+template<class Y>
+INLINE bool WeakPointerToBase<T>::
+owner_before(const WeakPointerToBase<Y> &other) const noexcept {
+  return _weak_ref < other._weak_ref;
+}
+
+/**
+ * Defines an ordering that is guaranteed to remain consistent even after this
+ * weak pointer has expired.  This may result in two pointers with the same
+ * get_orig() value comparing unequal if one of them is a new object that was
+ * allocated at the same memory address as the older, expired pointer.
+ */
+template<class T>
+template<class Y>
+INLINE bool WeakPointerToBase<T>::
+owner_before(const PointerToBase<Y> &other) const noexcept {
+  // Unfortunately, this may needlessly cause a control block to be allocated,
+  // but I do not see a more efficient solution.
+  return (other._void_ptr != nullptr) &&
+    (_void_ptr == nullptr || _weak_ref < ((const Y *)other._void_ptr)->get_weak_list());
+}
+
 /**
  * A convenient way to set the PointerTo object to NULL. (Assignment to a NULL
  * pointer also works, of course.)
@@ -539,9 +579,17 @@ template<class T>
 INLINE void WeakPointerToBase<T>::
 output(std::ostream &out) const {
   out << _void_ptr;
-  if (was_deleted()) {
-    out << ":deleted";
-  } else if (_void_ptr != nullptr) {
-    out << ":" << ((To *)_void_ptr)->get_ref_count();
+
+  WeakReferenceList *weak_ref = this->_weak_ref;
+  if (weak_ref != nullptr) {
+    weak_ref->_lock.lock();
+    if (!weak_ref->was_deleted()) {
+      out << ":" << ((To *)_void_ptr)->get_ref_count();
+    } else {
+      out << ":deleted";
+    }
+    weak_ref->_lock.unlock();
+  } else {
+    out << ":invalid";
   }
 }

panda/src/express/weakPointerToBase.h

@@ -89,6 +89,11 @@ public:
   INLINE bool operator < (const PointerToBase<To> &other) const;
 #endif  // CPPPARSER
 
+  template<class Y>
+  INLINE bool owner_before(const WeakPointerToBase<Y> &other) const noexcept;
+  template<class Y>
+  INLINE bool owner_before(const PointerToBase<Y> &other) const noexcept;
+
 PUBLISHED:
   INLINE void clear();
   INLINE void refresh() const;

panda/src/glstuff/glGeomMunger_src.cxx

@@ -426,11 +426,17 @@ premunge_format_impl(const GeomVertexFormat *orig) {
 int CLP(GeomMunger)::
 compare_to_impl(const GeomMunger *other) const {
   const CLP(GeomMunger) *om = (CLP(GeomMunger) *)other;
-  if (_texture != om->_texture) {
-    return _texture < om->_texture ? -1 : 1;
+  if (_texture.owner_before(om->_texture)) {
+    return -1;
   }
-  if (_tex_gen != om->_tex_gen) {
-    return _tex_gen < om->_tex_gen ? -1 : 1;
+  if (om->_texture.owner_before(_texture)) {
+    return 1;
+  }
+  if (_tex_gen.owner_before(om->_tex_gen)) {
+    return -1;
+  }
+  if (om->_tex_gen.owner_before(_tex_gen)) {
+    return 1;
   }
   if (_flags != om->_flags) {
     return _flags < om->_flags ? -1 : 1;
@@ -447,11 +453,17 @@ compare_to_impl(const GeomMunger *other) const {
 int CLP(GeomMunger)::
 geom_compare_to_impl(const GeomMunger *other) const {
   const CLP(GeomMunger) *om = (CLP(GeomMunger) *)other;
-  if (_texture != om->_texture) {
-    return _texture < om->_texture ? -1 : 1;
+  if (_texture.owner_before(om->_texture)) {
+    return -1;
   }
-  if (_tex_gen != om->_tex_gen) {
-    return _tex_gen < om->_tex_gen ? -1 : 1;
+  if (om->_texture.owner_before(_texture)) {
+    return 1;
+  }
+  if (_tex_gen.owner_before(om->_tex_gen)) {
+    return -1;
+  }
+  if (om->_tex_gen.owner_before(_tex_gen)) {
+    return 1;
   }
   if (_flags != om->_flags) {
     return _flags < om->_flags ? -1 : 1;

panda/src/pgraph/nodePath.cxx

@@ -5177,7 +5177,7 @@ get_stashed_ancestor(Thread *current_thread) const {
  */
 bool NodePath::
 operator == (const WeakNodePath &other) const {
-  return _head == other._head;
+  return (other == *this);
 }
 
 /**
@@ -5185,7 +5185,7 @@ operator == (const WeakNodePath &other) const {
  */
 bool NodePath::
 operator != (const WeakNodePath &other) const {
-  return _head != other._head;
+  return (other != *this);
 }
 
 /**
@@ -5196,7 +5196,7 @@ operator != (const WeakNodePath &other) const {
  */
 bool NodePath::
 operator < (const WeakNodePath &other) const {
-  return _head < other._head;
+  return other.compare_to(*this) > 0;
 }
 
 /**
@@ -5211,13 +5211,7 @@ operator < (const WeakNodePath &other) const {
  */
 int NodePath::
 compare_to(const WeakNodePath &other) const {
-  // Nowadays, the NodePathComponents at the head are pointerwise equivalent
-  // if and only if the NodePaths are equivalent.  So we only have to compare
-  // pointers.
-  if (_head != other._head) {
-    return _head < other._head ? -1 : 1;
-  }
-  return 0;
+  return -other.compare_to(*this);
 }
 
 /**

panda/src/pgraph/weakNodePath.I

@@ -124,7 +124,7 @@ node() const {
  */
 INLINE bool WeakNodePath::
 operator == (const NodePath &other) const {
-  return _head == other._head;
+  return _head.get_orig() == other._head && !_head.was_deleted();
 }
 
 /**
@@ -132,7 +132,7 @@ operator == (const NodePath &other) const {
  */
 INLINE bool WeakNodePath::
 operator != (const NodePath &other) const {
-  return _head != other._head;
+  return !operator == (other);
 }
 
 /**
@@ -143,7 +143,7 @@ operator != (const NodePath &other) const {
  */
 INLINE bool WeakNodePath::
 operator < (const NodePath &other) const {
-  return _head < other._head;
+  return _head.owner_before(other._head);
 }
 
 /**
@@ -158,8 +158,8 @@ operator < (const NodePath &other) const {
  */
 INLINE int WeakNodePath::
 compare_to(const NodePath &other) const {
-  if (_head != other._head) {
-    return _head < other._head ? -1 : 1;
+  if (operator != (other)) {
+    return _head.owner_before(other._head) ? -1 : 1;
   }
   return 0;
 }
@@ -170,7 +170,7 @@ compare_to(const NodePath &other) const {
  */
 INLINE bool WeakNodePath::
 operator == (const WeakNodePath &other) const {
-  return _head == other._head;
+  return !_head.owner_before(other._head) && !other._head.owner_before(_head);
 }
 
 /**
@@ -178,7 +178,7 @@ operator == (const WeakNodePath &other) const {
  */
 INLINE bool WeakNodePath::
 operator != (const WeakNodePath &other) const {
-  return _head != other._head;
+  return _head.owner_before(other._head) || other._head.owner_before(_head);
 }
 
 /**
@@ -189,7 +189,7 @@ operator != (const WeakNodePath &other) const {
  */
 INLINE bool WeakNodePath::
 operator < (const WeakNodePath &other) const {
-  return _head < other._head;
+  return _head.owner_before(other._head);
 }
 
 /**
@@ -204,10 +204,7 @@ operator < (const WeakNodePath &other) const {
  */
 INLINE int WeakNodePath::
 compare_to(const WeakNodePath &other) const {
-  if (_head != other._head) {
-    return _head < other._head ? -1 : 1;
-  }
-  return 0;
+  return other._head.owner_before(_head) - _head.owner_before(other._head);
 }
 
 /**