First attempt at providing default traits for PersistentValueMap.

First attempt at providing default traits for PersistentValueMap.

BUG=
R=dcarney@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=20038

[vogelheim, 2014-03-17 10:50:53]

This is a continuation of issue 195793024. Please have a look.

I'm not really sure where to go with this, so the two patch sets are essentially
two different proposals of where to go. The 2nd one looks nicer and is arguably
easier to use, at least when you get by with the provided default traits.

https://codereview.chromium.org/201643003/diff/20001/include/v8-util.h
File include/v8-util.h (right):

https://codereview.chromium.org/201643003/diff/20001/include/v8-util.h#newcode42
include/v8-util.h:42: typedef uintptr_t PersistentContainerValue;
This definition is a bit random... I guess it should go into PersistentValueMap,
since it really describes the implementation between PersistentValueMap and the
MapTraits?

https://codereview.chromium.org/201643003/diff/20001/include/v8-util.h#newcode46
include/v8-util.h:46: namespace util {
q: This namespace mainly serves to make the Traits classes available to users,
without polluting the main v8 namespace. Does that make sense? Or should the
entire PersistentValueMap go into this namespace?

[dcarney, 2014-03-17 12:53:48]

>
https://codereview.chromium.org/201643003/diff/20001/include/v8-util.h#newcode42
> include/v8-util.h:42: typedef uintptr_t PersistentContainerValue;
> This definition is a bit random... I guess it should go into
PersistentValueMap,
> since it really describes the implementation between PersistentValueMap and
the
> MapTraits?

first, you'll need it for PersistentValueVector
second, it would then be an unreachable definition, since you'd need to use it
like PersistentValueMap<K, V, T>::PersistentContainerValue in the definition of
T, which C++ dislikes.

[dcarney, 2014-03-17 12:53:55]

https://codereview.chromium.org/201643003/diff/60001/include/v8-util.h
File include/v8-util.h (right):

https://codereview.chromium.org/201643003/diff/60001/include/v8-util.h#newcode46
include/v8-util.h:46: namespace persistent_value_map_traits {
no need for a new namespace

https://codereview.chromium.org/201643003/diff/60001/include/v8-util.h#newcode49
include/v8-util.h:49: class WeakSTLMapTraits {
would be better as StdPersistentValueMap  which itself has weak traits, which
default to something which does nothing

https://codereview.chromium.org/201643003/diff/60001/include/v8-util.h#newcode86
include/v8-util.h:86: static const bool kIsWeak = false;
this should be true for a class called WeakSTLMapTraits

[vogelheim, 2014-03-17 19:20:37]

PTAL.

https://codereview.chromium.org/201643003/diff/60001/include/v8-util.h
File include/v8-util.h (right):

https://codereview.chromium.org/201643003/diff/60001/include/v8-util.h#newcode46
include/v8-util.h:46: namespace persistent_value_map_traits {
On 2014/03/17 12:53:55, dcarney wrote:
> no need for a new namespace

Removed.

https://codereview.chromium.org/201643003/diff/60001/include/v8-util.h#newcode49
include/v8-util.h:49: class WeakSTLMapTraits {
On 2014/03/17 12:53:55, dcarney wrote:
> would be better as StdPersistentValueMap  which itself has weak traits, which
> default to something which does nothing

I couldn't really figure this one out. What I've done now is:

- Fixed the K() issue, so that the (optional) Traits methods with return values
can always be implemented as 'return NULL'.
- Renamed the Traits class to be more consistent with its actual role:
DefaultSTLMapTraits.

The test-api.cc demonstrates how to use the traits class as-is, and how to
implement a map w/ weak callbacks by deriving a custom traits class from the
default one.

Let's briefly discuss whether that addresses your concerns.

https://codereview.chromium.org/201643003/diff/60001/include/v8-util.h#newcode86
include/v8-util.h:86: static const bool kIsWeak = false;
On 2014/03/17 12:53:55, dcarney wrote:
> this should be true for a class called WeakSTLMapTraits

Yes. But actually, this class was misnamed, since it didn't actually implement a
'weak' maps trait. So I renamed the class instead.

[vogelheim, 2014-03-18 10:47:54]

It turns out I was being daft, and removing the default implementations works as
it should. :-|

I guess the remaining question is whether we stay with one Traits class, or try
to some split this up?

[vogelheim, 2014-03-18 12:05:54]

PTAL. I think this should address the review comments.

[dcarney, 2014-03-18 12:13:19]

looking good, i still don't like the problem with the overrides

https://codereview.chromium.org/201643003/diff/120001/include/v8-util.h
File include/v8-util.h (right):

https://codereview.chromium.org/201643003/diff/120001/include/v8-util.h#newco...
include/v8-util.h:87: static void Dispose(v8::Isolate* isolate,
v8::UniquePersistent<V> value,
still have the problem that these are overridden in the sublclass

https://codereview.chromium.org/201643003/diff/120001/include/v8-util.h#newco...
include/v8-util.h:102: /**
2 lines between definitions.

https://codereview.chromium.org/201643003/diff/120001/include/v8-util.h#newco...
include/v8-util.h:269: /**
again

[vogelheim, 2014-03-18 12:55:12]

https://codereview.chromium.org/201643003/diff/120001/include/v8-util.h
File include/v8-util.h (right):

https://codereview.chromium.org/201643003/diff/120001/include/v8-util.h#newco...
include/v8-util.h:87: static void Dispose(v8::Isolate* isolate,
v8::UniquePersistent<V> value,
On 2014/03/18 12:13:19, dcarney wrote:
> still have the problem that these are overridden in the sublclass

Done.

Now:
- StdMapTraits: std::map as backing impl.
- StrongMapTraits: StdMapTraits + kIsWeak = false
- DefaultPersistentValueMapTraits: StrongMapTraits + Dispose.

https://codereview.chromium.org/201643003/diff/120001/include/v8-util.h#newco...
include/v8-util.h:102: /**
On 2014/03/18 12:13:19, dcarney wrote:
> 2 lines between definitions.

Done.

https://codereview.chromium.org/201643003/diff/120001/include/v8-util.h#newco...
include/v8-util.h:269: /**
On 2014/03/18 12:13:19, dcarney wrote:
> again

Done.

[dcarney, 2014-03-18 13:48:52]

great, lgtm

[dcarney, 2014-03-18 13:48:52]

great, lgtm

[dcarney, 2014-03-18 15:01:18]

Committed patchset #9 manually as r20038 (presubmit successful).