From 243ee75e47563def6620c3cfebf6dcc12f4c9b31 Mon Sep 17 00:00:00 2001
From: rdb <git@rdb.name>
Date: Mon, 30 Jul 2018 22:24:55 +0200
Subject: [PATCH] express: refactor WeakPointerTo::lock() a little bit

---
 panda/src/express/weakPointerTo.I     | 40 ++++-----------------------
 panda/src/express/weakPointerToBase.I | 28 +++++++++++++++++++
 panda/src/express/weakPointerToBase.h |  2 ++
 panda/src/express/weakPointerToVoid.I |  8 ++++--
 4 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/panda/src/express/weakPointerTo.I b/panda/src/express/weakPointerTo.I
index 354846807a..8d60057c31 100644
--- a/panda/src/express/weakPointerTo.I
+++ b/panda/src/express/weakPointerTo.I
@@ -125,23 +125,9 @@ operator T * () const {
 template<class T>
 INLINE PointerTo<T> WeakPointerTo<T>::
 lock() const {
-  WeakReferenceList *weak_ref = this->_weak_ref;
-  if (weak_ref != nullptr) {
-    PointerTo<T> ptr;
-    weak_ref->_lock.lock();
-    if (!weak_ref->was_deleted()) {
-      // We also need to check that the reference count is not zero (which can
-      // happen if the object is currently being destructed), since that could
-      // cause double deletion.
-      To *plain_ptr = (To *)WeakPointerToBase<T>::_void_ptr;
-      if (plain_ptr != nullptr && plain_ptr->ref_if_nonzero()) {
-        ptr.cheat() = plain_ptr;
-      }
-    }
-    weak_ref->_lock.unlock();
-    return ptr;
-  }
-  return nullptr;
+  PointerTo<T> ptr;
+  this->lock_into(ptr);
+  return ptr;
 }
 
 /**
@@ -415,23 +401,9 @@ operator const T * () const {
 template<class T>
 INLINE ConstPointerTo<T> WeakConstPointerTo<T>::
 lock() const {
-  WeakReferenceList *weak_ref = this->_weak_ref;
-  if (weak_ref != nullptr) {
-    ConstPointerTo<T> ptr;
-    weak_ref->_lock.lock();
-    if (!weak_ref->was_deleted()) {
-      // We also need to check that the reference count is not zero (which can
-      // happen if the object is currently being destructed), since that could
-      // cause double deletion.
-      const To *plain_ptr = (const To *)WeakPointerToBase<T>::_void_ptr;
-      if (plain_ptr != nullptr && plain_ptr->ref_if_nonzero()) {
-        ptr.cheat() = plain_ptr;
-      }
-    }
-    weak_ref->_lock.unlock();
-    return ptr;
-  }
-  return nullptr;
+  ConstPointerTo<T> ptr;
+  this->lock_into(ptr);
+  return ptr;
 }
 
 /**
diff --git a/panda/src/express/weakPointerToBase.I b/panda/src/express/weakPointerToBase.I
index c57ea31719..8c6de65523 100644
--- a/panda/src/express/weakPointerToBase.I
+++ b/panda/src/express/weakPointerToBase.I
@@ -238,6 +238,34 @@ update_type(To *ptr) {
 #endif  // DO_MEMORY_USAGE
 }
 
+/**
+ * A thread-safe way to access the underlying pointer; will only write to the
+ * given pointer if the underlying pointer has not yet been deleted and is not
+ * null.  Note that it may leave the pointer unassigned even if was_deleted()
+ * still returns true, which can occur if the object has reached reference
+ * count 0 and is about to be destroyed.
+ */
+template<class T>
+INLINE void WeakPointerToBase<T>::
+lock_into(PointerToBase<To> &locked) const {
+  WeakReferenceList *weak_ref = this->_weak_ref;
+  if (weak_ref != nullptr) {
+    weak_ref->_lock.lock();
+    if (!weak_ref->was_deleted()) {
+      // We also need to check that the reference count is not zero (which can
+      // happen if the object is currently being destructed), since that could
+      // cause double deletion.
+      To *plain_ptr = (To *)WeakPointerToBase<T>::_void_ptr;
+      if (plain_ptr != nullptr && plain_ptr->ref_if_nonzero()) {
+        // It is valid and we successfully grabbed a reference.  Assign it,
+        // noting we have already incremented the reference count.
+        locked._void_ptr = plain_ptr;
+      }
+    }
+    weak_ref->_lock.unlock();
+  }
+}
+
 #ifndef CPPPARSER
 /**
  *
diff --git a/panda/src/express/weakPointerToBase.h b/panda/src/express/weakPointerToBase.h
index 60be402c4c..474514b055 100644
--- a/panda/src/express/weakPointerToBase.h
+++ b/panda/src/express/weakPointerToBase.h
@@ -47,6 +47,8 @@ protected:
 
   INLINE void update_type(To *ptr);
 
+  INLINE void lock_into(PointerToBase<To> &locked) const;
+
   // No assignment or retrieval functions are declared in WeakPointerToBase,
   // because we will have to specialize on const vs.  non-const later.
 
diff --git a/panda/src/express/weakPointerToVoid.I b/panda/src/express/weakPointerToVoid.I
index 187a659fc1..83253c7921 100644
--- a/panda/src/express/weakPointerToVoid.I
+++ b/panda/src/express/weakPointerToVoid.I
@@ -40,7 +40,11 @@ remove_callback(WeakPointerCallback *callback) const {
 
 /**
  * Returns true if the object we are pointing to has been deleted, false
- * otherwise.
+ * otherwise.  If this returns true, it means that the pointer can not yet be
+ * reused, but it does not guarantee that it can be safely accessed.  See the
+ * lock() method for a safe way to access the underlying pointer.
+ *
+ * This will always return true for a null pointer, unlike is_valid_pointer().
  */
 INLINE bool WeakPointerToVoid::
 was_deleted() const {
@@ -49,7 +53,7 @@ was_deleted() const {
 
 /**
  * Returns true if the pointer is not null and the object has not been
- * deleted.
+ * deleted.  See was_deleted() for caveats.
  */
 INLINE bool WeakPointerToVoid::
 is_valid_pointer() const {