Amend PersistentValueMap.

include/v8-util.h

Index: include/v8-util.h
diff --git a/include/v8-util.h b/include/v8-util.h
index 3f8cc6d269d0a6598944720980771210de49b73c..597fe43d3194e32efd15b4090dcf822e0f0eeac1 100644
--- a/include/v8-util.h
+++ b/include/v8-util.h
@@ -92,38 +92,34 @@ class StdMapTraits {
 /**
  * A default trait implementation for PersistentValueMap, which inherits
  * a std:map backing map from StdMapTraits and holds non-weak persistent
- * objects.
+ * objects and has no special Dispose handling.
  *
- * Users have to implement their own dispose trait.
+ * You should not derive from this class, since MapType depends on the
+ * surrounding class, and hence a subclass cannot simply inherit the methods.
  */
 template<typename K, typename V>
-class StrongMapTraits : public StdMapTraits<K, V> {
+class DefaultPersistentValueMapTraits : public StdMapTraits<K, V> {
  public:
   // Weak callback & friends:
   static const bool kIsWeak = false;
-  typedef typename StdMapTraits<K, V>::Impl Impl;
+  typedef PersistentValueMap<K, V, DefaultPersistentValueMapTraits<K, V> >
+      MapType;
   typedef void WeakCallbackDataType;
+
   static WeakCallbackDataType* WeakCallbackParameter(
-      Impl* impl, const K& key, Local<V> value);
-  static Impl* ImplFromWeakCallbackData(
-      const WeakCallbackData<V, WeakCallbackDataType>& data);
+      MapType* map, const K& key, Local<V> value) {
+    return NULL;
+  }
+  static MapType* MapFromWeakCallbackData(
+          const WeakCallbackData<V, WeakCallbackDataType>& data) {
+    return NULL;
+  }
   static K KeyFromWeakCallbackData(
-      const WeakCallbackData<V, WeakCallbackDataType>& data);
-  static void DisposeCallbackData(WeakCallbackDataType* data);
-};
-
-
-/**
- * A default trait implementation for PersistentValueMap, with a std::map
- * backing map, non-weak persistents as values, and no special dispose
- * handling. Can be used as-is.
- */
-template<typename K, typename V>
-class DefaultPersistentValueMapTraits : public StrongMapTraits<K, V> {
- public:
-  typedef typename StrongMapTraits<K, V>::Impl Impl;
-  static void Dispose(Isolate* isolate, UniquePersistent<V> value,
-      Impl* impl, K key) { }
+      const WeakCallbackData<V, WeakCallbackDataType>& data) {
+    return K();
+  }
+  static void DisposeCallbackData(WeakCallbackDataType* data) { }
+  static void Dispose(Isolate* isolate, UniquePersistent<V> value, K key) { }
 };
 
 
@@ -167,7 +163,7 @@ class PersistentValueMap {
    * Check whether a value is contained in the map.
    */
   V8_INLINE bool Contains(const K& key) {
-    return Traits::Get(&impl_, key) != 0;
+    return Traits::Get(&impl_, key) != kPersistentContainerNotFound;
   }
 
   /**
@@ -175,14 +171,8 @@ class PersistentValueMap {
    * Return true if a value was found.
    */
   V8_INLINE bool SetReturnValue(const K& key,
-      ReturnValue<Value>& returnValue) {
-    PersistentContainerValue value = Traits::Get(&impl_, key);
-    bool hasValue = value != 0;
-    if (hasValue) {
-      returnValue.SetInternal(
-          *reinterpret_cast<internal::Object**>(FromVal(value)));
-    }
-    return hasValue;
+      ReturnValue<Value> returnValue) {
+    return SetReturnValueFromVal(returnValue, Traits::Get(&impl_, key));
   }
 
   /**
@@ -231,12 +221,71 @@ class PersistentValueMap {
       typename Traits::Impl impl;
       Traits::Swap(impl_, impl);
       for (It i = Traits::Begin(&impl); i != Traits::End(&impl); ++i) {
-        Traits::Dispose(isolate_, Release(Traits::Value(i)).Pass(), &impl,
-          Traits::Key(i));
+        Traits::Dispose(isolate_, Release(Traits::Value(i)).Pass(),
+                        Traits::Key(i));
       }
     }
   }
 
+  /**
+   * Helper class for GetReference/SetWithReference. Do not use outside
+   * that context.
+   */
+  class PersistentValueReference {
+   public:
+    PersistentValueReference() : value_(kPersistentContainerNotFound) { }
+    Local<V> NewLocal(Isolate* isolate) const {
+      return Local<V>::New(isolate, FromVal(value_));
+    }
+    bool IsEmpty() const {
+      return value_ == kPersistentContainerNotFound;
+    }
+    template<typename T>
+    bool SetReturnValue(ReturnValue<T> returnValue) {
+      return SetReturnValueFromVal(returnValue, value_);
+    }
+    void Reset() {
+      value_ = kPersistentContainerNotFound;
+    }
+    void operator=(const PersistentValueReference& other) {
+      value_ = other.value_;
+    }
+
+   private:
+    friend class PersistentValueMap;
+    explicit PersistentValueReference(PersistentContainerValue value)

[dcarney, 2014/03/27 07:45:22]

expose copy constructor

[vogelheim, 2014/03/27 14:43:54]

Done.

+        : value_(value) { }
+
+    PersistentContainerValue value_;
+  };
+
+  /**
+   * Helper for V8ValueCache: Get a reference to a map value.

[dcarney, 2014/03/27 07:45:22]

don't mention v8valuecache

[vogelheim, 2014/03/27 14:43:54]

Done.

+   * This enables fast, repeated access to a value stored in the map
+   * while the map remains unchanged.
+   *
+   * Careful: This is potentially unsafe, in that *any* change to the

[dcarney, 2014/03/27 07:45:22]

there are limited actions that invalidates these : calling Set or Remove with
the same key, the weak callback for the key, and Clear()

mention them explicitly

[vogelheim, 2014/03/27 14:43:54]

Done.

I'm not entirely convinced of it, though: This is the type of comment that has a
high chance of becoming stale/incorrect, in that changes to the Map may
invalidate the given list of specific functions. Since this comment isn't
located where those changes would happen, the chance that the comment gets
updated is rather slim. Hence my preference for a stern but somewhat unspecific
warning.

+   * map may invalidate the reference.
+   */
+  V8_INLINE PersistentValueReference GetReference(const K& key) {
+    return PersistentValueReference(Traits::Get(&impl_, key));
+  }
+
+  /**
+   * Helper for V8ValueCache: Set a new persistent in the map, and
+   * also return a reference to it.
+   * This enables fast, repeated access to a value stored in the map
+   * while the map remains unchanged.
+   *
+   * Careful: This is potentially unsafe, in that *any* change to the
+   * map may invalidate the reference.
+   */
+  V8_INLINE PersistentValueReference SetWithReference(

[dcarney, 2014/03/27 07:45:22]

no need for this function since it calls just Set followed by Get

we could instead have a function
UniquePersistent Set(key, value, ValueReference*)
that does all the work of regular Set but sets the valuereference before the
actual call to Set

[vogelheim, 2014/03/27 14:43:54]

Removed.
Done. Added tests.

+      const K& key, UniquePersistent<V> value) {
+    Set(key, value.Pass());
+    return GetReference(key);
+  }
+
  private:
   PersistentValueMap(PersistentValueMap&);
   void operator=(PersistentValueMap&);
@@ -249,7 +298,7 @@ class PersistentValueMap {
     if (Traits::kIsWeak) {
       Local<V> value(Local<V>::New(isolate_, *persistent));
       persistent->template SetWeak<typename Traits::WeakCallbackDataType>(
-        Traits::WeakCallbackParameter(&impl_, key, value), WeakCallback);
+        Traits::WeakCallbackParameter(this, key, value), WeakCallback);
     }
     PersistentContainerValue old_value =
         Traits::Set(&impl_, key, ClearAndLeak(persistent));
@@ -259,10 +308,11 @@ class PersistentValueMap {
   static void WeakCallback(
       const WeakCallbackData<V, typename Traits::WeakCallbackDataType>& data) {
     if (Traits::kIsWeak) {
-      typename Traits::Impl* impl = Traits::ImplFromWeakCallbackData(data);
+      PersistentValueMap<K, V, Traits>* persistentValueMap =
+          Traits::MapFromWeakCallbackData(data);
       K key = Traits::KeyFromWeakCallbackData(data);
-      PersistentContainerValue value = Traits::Remove(impl, key);
-      Traits::Dispose(data.GetIsolate(), Release(value).Pass(), impl, key);
+      Traits::Dispose(data.GetIsolate(),
+                      persistentValueMap->Remove(key).Pass(), key);
     }
   }
 
@@ -270,6 +320,16 @@ class PersistentValueMap {
     return reinterpret_cast<V*>(v);
   }
 
+  V8_INLINE static bool SetReturnValueFromVal(
+      ReturnValue<Value>& returnValue, PersistentContainerValue value) {
+    bool hasValue = value != kPersistentContainerNotFound;
+    if (hasValue) {
+      returnValue.SetInternal(
+          *reinterpret_cast<internal::Object**>(FromVal(value)));
+    }
+    return hasValue;
+  }
+
   V8_INLINE static PersistentContainerValue ClearAndLeak(
       UniquePersistent<V>* persistent) {
     V* v = persistent->val_;
@@ -312,44 +372,6 @@ class StdPersistentValueMap : public PersistentValueMap<K, V, Traits> {
       : PersistentValueMap<K, V, Traits>(isolate) {}
 };
 
-
-/**
- * Empty default implementations for StrongTraits methods.
- *
- * These should not be necessary, since they're only used in code that
- * is surrounded by if(Traits::kIsWeak), which for StrongMapTraits is
- * compile-time false. Most compilers can live without them; however
- * the compiler we use from 64-bit Win differs.
- *
- * TODO(vogelheim): Remove these once they're no longer necessary.
- */
-template<typename K, typename V>
-typename StrongMapTraits<K, V>::WeakCallbackDataType*
-    StrongMapTraits<K, V>::WeakCallbackParameter(
-        Impl* impl, const K& key, Local<V> value) {
-  return NULL;
-}
-
-
-template<typename K, typename V>
-typename StrongMapTraits<K, V>::Impl*
-    StrongMapTraits<K, V>::ImplFromWeakCallbackData(
-        const WeakCallbackData<V, WeakCallbackDataType>& data) {
-  return NULL;
-}
-
-
-template<typename K, typename V>
-K StrongMapTraits<K, V>::KeyFromWeakCallbackData(
-    const WeakCallbackData<V, WeakCallbackDataType>& data) {
-  return K();
-}
-
-
-template<typename K, typename V>
-void StrongMapTraits<K, V>::DisposeCallbackData(WeakCallbackDataType* data) {
-}
-
 }  // namespace v8
 
 #endif  // V8_UTIL_H_