[v8-dev] Re: Allow hidden properties and implement GetIdentityHash

2009-03-09 Thread mbelshe
I think this will work. My only concern is the "...hidden..."; but I think you already plan to address that separately. http://codereview.chromium.org/40324/diff/1/4 File src/api.cc (right): http://codereview.chromium.org/40324/diff/1/4#newcode1920 Line 1920: static_cast(None)); use ReadOnly?

[v8-dev] Re: Allow hidden properties and implement GetIdentityHash

2009-03-09 Thread mbelshe
Looks good - My only concern is the "...hidden...", but I think you plan to address that separately. http://codereview.chromium.org/40324/diff/1/4 File src/api.cc (right): http://codereview.chromium.org/40324/diff/1/4#newcode1920 Line 1920: static_cast(None)); use ReadOnly? http://codereview.

[v8-dev] Re: Allow hidden properties and implement GetIdentityHash

2009-03-09 Thread mbelshe
Looks good - My only concern is the "...hidden...", but I think you plan to address that separately. http://codereview.chromium.org/40324/diff/1/4 File src/api.cc (right): http://codereview.chromium.org/40324/diff/1/4#newcode1920 Line 1920: static_cast(None)); use ReadOnly? http://codereview.

[v8-dev] Re: Allow hidden properties and implement GetIdentityHash

2009-03-09 Thread kasperl
LGTM. My only real concern is the timing of the commit; I'd prefer not to get this pushed to trunk as part of the 1.0.4 push that I hope we can land real soon (blocked on issue 265). http://codereview.chromium.org/40324/diff/1/4 File src/api.cc (right): http://codereview.chromium.org/40324/diff

[v8-dev] Re: Allow hidden properties and implement GetIdentityHash

2009-03-10 Thread ager
Overall, this looks good. I agree with Kasper on the timing. We need to get the new compiler pushed to trunk before landing this in bleeding_edge. Once we land this in bleeding_edge, we need to make sure that the real hiding of the property happens fairly quickly so we are not blocked from push

[v8-dev] Re: Allow hidden properties and implement GetIdentityHash

2009-03-19 Thread kasperl
LGTM. http://codereview.chromium.org/50016/diff/1/2 File test/cctest/test-api.cc (right): http://codereview.chromium.org/50016/diff/1/2#newcode1267 Line 1267: CHECK_NE(hash, hash2); Is this really guaranteed? Freaky. http://codereview.chromium.org/50016/diff/1/2#newcode1270 Line 1270: CHECK_NE

[v8-dev] Re: Allow hidden properties and implement GetIdentityHash

2009-03-19 Thread iposva
http://codereview.chromium.org/50016/diff/1/2 File test/cctest/test-api.cc (right): http://codereview.chromium.org/50016/diff/1/2#newcode1267 Line 1267: CHECK_NE(hash, hash2); On 2009/03/19 17:46:03, Kasper Lund wrote: > Is this really guaranteed? Freaky. I will add a comment that if two consecu

[v8-dev] Re: Allow hidden properties and implement GetIdentityHash

2009-03-19 Thread ager
LGTM http://codereview.chromium.org/50016/diff/1/3 File include/v8.h (right): http://codereview.chromium.org/50016/diff/1/3#newcode1080 Line 1080: * Returns the identity hash for this object. We should state in the API that one of the hidden value keys is used by this call (the potential for co

[v8-dev] Re: Allow hidden properties and implement GetIdentityHash

2009-03-19 Thread iposva
http://codereview.chromium.org/50016/diff/1/3 File include/v8.h (right): http://codereview.chromium.org/50016/diff/1/3#newcode1080 Line 1080: * Returns the identity hash for this object. On 2009/03/19 18:21:44, Mads Ager wrote: > We should state in the API that one of the hidden value keys is us