Amend PersistentValueMap.

include/v8-util.h

 // Copyright 2014 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 74 matching lines @@
     PersistentContainerValue value = it->second;
     impl->erase(it);
     return value;
   }
 };
 
 
 /**
  * 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);
+      const WeakCallbackData<V, WeakCallbackDataType>& data) {
+    return K();
+  }
+  static void DisposeCallbackData(WeakCallbackDataType* data) { }
+  static void Dispose(Isolate* isolate, UniquePersistent<V> value, K key) { }
 };
 
 
-/**
- * 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) { }
-};
-
-
 /**
  * A map wrapper that allows using UniquePersistent as a mapped value.
  * C++11 embedders don't need this class, as they can use UniquePersistent
  * directly in std containers.
  *
  * The map relies on a backing map, whose type and accessors are described
  * by the Traits class. The backing map will handle values of type
  * PersistentContainerValue, with all conversion into and out of V8
  * handles being transparently handled by this class.
  */
@@ skipping 20 matching lines @@
    * Get value stored in map.
    */
   V8_INLINE Local<V> Get(const K& key) {
     return Local<V>::New(isolate_, FromVal(Traits::Get(&impl_, key)));
   }
 
   /**
    * 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;
   }
 
   /**
    * Get value stored in map and set it in returnValue.
    * 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));
   }
 
   /**
    * Call Isolate::SetReference with the given parent and the map value.
    */
   V8_INLINE void SetReference(const K& key,
       const Persistent<Object>& parent) {
     GetIsolate()->SetReference(
       reinterpret_cast<internal::Object**>(parent.val_),
       reinterpret_cast<internal::Object**>(FromVal(Traits::Get(&impl_, key))));
@@ skipping 28 matching lines @@
   * in case side effects of disposal cause insertions.
   **/
   void Clear() {
     typedef typename Traits::Iterator It;
     HandleScope handle_scope(isolate_);
     // TODO(dcarney): figure out if this swap and loop is necessary.
     while (!Traits::Empty(&impl_)) {
       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&);
 
   /**
    * Put the value into the map, and set the 'weak' callback when demanded
    * by the Traits class.
    */
   UniquePersistent<V> SetUnique(const K& key, UniquePersistent<V>* persistent) {
     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));
     return Release(old_value).Pass();
   }
 
   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);
     }
   }
 
   V8_INLINE static V* FromVal(PersistentContainerValue v) {
     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_;
     persistent->val_ = 0;
     return reinterpret_cast<PersistentContainerValue>(v);
   }
 
   /**
    * Return a container value as UniquePersistent and make sure the weak
    * callback is properly disposed of. All remove functionality should go
@@ skipping 22 matching lines @@
  * UniquePersistent directly in std containers.
  */
 template<typename K, typename V,
     typename Traits = DefaultPersistentValueMapTraits<K, V> >
 class StdPersistentValueMap : public PersistentValueMap<K, V, Traits> {
  public:
   explicit StdPersistentValueMap(Isolate* isolate)
       : 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_