Incorporate improvements from https://github.com/drses/weak-map/pull/3/files

At https://github.com/drses/weak-map/pull/3/files Kris Kowal sped up
WeakMaps by using the hiddenRecord directly as the array-like, indexed
by a per-map unique index.

I kept the old defProp that explicitly enumerates the writable,
enumerable, configurable attributes that should default to false
anyway, out of general paranoia about remaining undiagnosed bugs in
Object.defineProperty.

[MarkM, 2014-01-21 00:50:37]

At https://github.com/drses/weak-map/pull/3/files Kris Kowal sped up
WeakMaps by using the hiddenRecord directly as the array-like, indexed
by a per-map unique index.

I kept the old defProp that explicitly enumerates the writable,
enumerable, configurable attributes that should default to false
anyway, out of general paranoia about remaining undiagnosed bugs in
Object.defineProperty.

[kpreid2, 2014-01-21 18:13:14]

https://codereview.appspot.com/54750043/diff/20001/src/com/google/caja/ses/We...
File src/com/google/caja/ses/WeakMap.js (right):

https://codereview.appspot.com/54750043/diff/20001/src/com/google/caja/ses/We...
src/com/google/caja/ses/WeakMap.js:313: var hiddenRecord = Object.create(null);
Note that Object.create(null) is not available on Chrome versions near
28.0.1480.0 due to the __proto__ breakage.

If this is still a concern, we must be able to fall back to a regular {} here.

If it is obsolete, then we should (separately) remove all of the kludges that
were added in r5359.

https://codereview.appspot.com/54750043/diff/20001/src/com/google/caja/ses/We...
src/com/google/caja/ses/WeakMap.js:426: function delete___(key) {
Note that per ES6-draft this should return whether the key originally existed.
The previous implementation returned true always and this one is inconsistent.

We should start having tests for WeakMap; it would be reasonable to add them to
test-ses-parts.js for now.

https://codereview.appspot.com/54750043/diff/20001/src/com/google/caja/ses/We...
src/com/google/caja/ses/WeakMap.js:431: return keys.indexOf(key) >= 0;
This doesn't implement any deletion; it appears to be a copy of has___.

Please add a test that would have caught this bug.

[MarkM, 2014-01-24 05:22:59]

At https://github.com/drses/weak-map/pull/3/files Kris Kowal sped up
WeakMaps by using the hiddenRecord directly as the array-like, indexed
by a per-map unique index.

I kept the old defProp that explicitly enumerates the writable,
enumerable, configurable attributes that should default to false
anyway, out of general paranoia about remaining undiagnosed bugs in
Object.defineProperty.

[MarkM, 2014-01-24 05:23:24]

At https://github.com/drses/weak-map/pull/3/files Kris Kowal sped up
WeakMaps by using the hiddenRecord directly as the array-like, indexed
by a per-map unique index.

I kept the old defProp that explicitly enumerates the writable,
enumerable, configurable attributes that should default to false
anyway, out of general paranoia about remaining undiagnosed bugs in
Object.defineProperty.

[MarkM, 2014-01-24 05:24:07]

https://codereview.appspot.com/54750043/diff/20001/src/com/google/caja/ses/We...
File src/com/google/caja/ses/WeakMap.js (right):

https://codereview.appspot.com/54750043/diff/20001/src/com/google/caja/ses/We...
src/com/google/caja/ses/WeakMap.js:313: var hiddenRecord = Object.create(null);
On 2014/01/21 18:13:15, kpreid2 wrote:
> Note that Object.create(null) is not available on Chrome versions near
> 28.0.1480.0 due to the __proto__ breakage.
> 
> If this is still a concern, we must be able to fall back to a regular {} here.
> 
> If it is obsolete, then we should (separately) remove all of the kludges that
> were added in r5359.

Done.

https://codereview.appspot.com/54750043/diff/20001/src/com/google/caja/ses/We...
src/com/google/caja/ses/WeakMap.js:426: function delete___(key) {
On 2014/01/21 18:13:15, kpreid2 wrote:
> Note that per ES6-draft this should return whether the key originally existed.
> The previous implementation returned true always and this one is inconsistent.
> 
> We should start having tests for WeakMap; it would be reasonable to add them
to
> test-ses-parts.js for now.

Since this is based on Kris's weak-map, I'm checking with him at
https://github.com/drses/weak-map/issues/5 and will revise accordingly. But your
diagnosis seems correct.

https://codereview.appspot.com/54750043/diff/20001/src/com/google/caja/ses/We...
src/com/google/caja/ses/WeakMap.js:431: return keys.indexOf(key) >= 0;
On 2014/01/21 18:13:15, kpreid2 wrote:
> This doesn't implement any deletion; it appears to be a copy of has___.
> 
> Please add a test that would have caught this bug.

Since this is based on Kris's weak-map, I'm checking with him at
https://github.com/drses/weak-map/issues/5 and will revise accordingly. But your
diagnosis seems correct.

[MarkM, 2014-01-24 05:33:46]

At https://github.com/drses/weak-map/pull/3/files Kris Kowal sped up
WeakMaps by using the hiddenRecord directly as the array-like, indexed
by a per-map unique index.

I kept the old defProp that explicitly enumerates the writable,
enumerable, configurable attributes that should default to false
anyway, out of general paranoia about remaining undiagnosed bugs in
Object.defineProperty.

[MarkM, 2014-01-24 19:26:11]

At https://github.com/drses/weak-map/pull/3/files Kris Kowal sped up
WeakMaps by using the hiddenRecord directly as the array-like, indexed
by a per-map unique index.

I kept the old defProp that explicitly enumerates the writable,
enumerable, configurable attributes that should default to false
anyway, out of general paranoia about remaining undiagnosed bugs in
Object.defineProperty.

[MarkM, 2014-01-24 22:26:36]

At https://github.com/drses/weak-map/pull/3/files Kris Kowal sped up
WeakMaps by using the hiddenRecord directly as the array-like, indexed
by a per-map unique index.

I kept the old defProp that explicitly enumerates the writable,
enumerable, configurable attributes that should default to false
anyway, out of general paranoia about remaining undiagnosed bugs in
Object.defineProperty.

[MarkM, 2014-01-24 22:28:03]

Ready

https://codereview.appspot.com/54750043/diff/20001/src/com/google/caja/ses/We...
File src/com/google/caja/ses/WeakMap.js (right):

https://codereview.appspot.com/54750043/diff/20001/src/com/google/caja/ses/We...
src/com/google/caja/ses/WeakMap.js:426: function delete___(key) {
On 2014/01/21 18:13:15, kpreid2 wrote:
> Note that per ES6-draft this should return whether the key originally existed.
> The previous implementation returned true always and this one is inconsistent.
> 
> We should start having tests for WeakMap; it would be reasonable to add them
to
> test-ses-parts.js for now.

Done.

https://codereview.appspot.com/54750043/diff/20001/src/com/google/caja/ses/We...
src/com/google/caja/ses/WeakMap.js:431: return keys.indexOf(key) >= 0;
On 2014/01/21 18:13:15, kpreid2 wrote:
> This doesn't implement any deletion; it appears to be a copy of has___.
> 
> Please add a test that would have caught this bug.

Done.

[MarkM, 2014-01-24 22:31:26]

At https://github.com/drses/weak-map/pull/3/files Kris Kowal sped up
WeakMaps by using the hiddenRecord directly as the array-like, indexed
by a per-map unique index.

I kept the old defProp that explicitly enumerates the writable,
enumerable, configurable attributes that should default to false
anyway, out of general paranoia about remaining undiagnosed bugs in
Object.defineProperty.

[kpreid2, 2014-01-24 23:00:30]

LGTM

https://codereview.appspot.com/54750043/diff/130003/src/com/google/caja/ses/W...
File src/com/google/caja/ses/WeakMap.js (right):

https://codereview.appspot.com/54750043/diff/130003/src/com/google/caja/ses/W...
src/com/google/caja/ses/WeakMap.js:445: keys.push(key);
If I were feeling especially paranoid (considering such things as
slow-script-stopping), I might check here the invariant that keys and values are
the same length, since a violation would result in mis-associating all future
keys and values, a particularly horrible outcome.

https://codereview.appspot.com/54750043/diff/130003/src/com/google/caja/ses/W...
src/com/google/caja/ses/WeakMap.js:461: values.splice(index, 1);
Splicing is probably O(n); instead, move the last entry into this index. Since
WeakMaps are non-enumerable the ordering makes no observable difference.

(Should you choose to adopt the abovementioned paranoia, erase either the key or
value of the destination, copy the other, and copy the blanked value.)

https://codereview.appspot.com/54750043/diff/130003/tests/com/google/caja/ses...
File tests/com/google/caja/ses/test-ses-parts.js (right):

https://codereview.appspot.com/54750043/diff/130003/tests/com/google/caja/ses...
tests/com/google/caja/ses/test-ses-parts.js:15: var preFrozen =
Object.freeze({});
+1 for thoroughness

https://codereview.appspot.com/54750043/diff/130003/tests/com/google/caja/ses...
tests/com/google/caja/ses/test-ses-parts.js:38: var wm1 = new WeakMap();
Either give these more descriptive names or add comments explaining which roles
they play in the test, so that one doesn't have to figure out the logic of the
tests by reading all the asserts.

https://codereview.appspot.com/54750043/diff/130003/tests/com/google/caja/ses...
tests/com/google/caja/ses/test-ses-parts.js:43: assertEquals(wm1.set(postFrozen,
10), wm1);
Expected values go first, actual values go second. Jsunit error messages are
written to assume that.

[MarkM, 2014-01-25 04:01:42]

At https://github.com/drses/weak-map/pull/3/files Kris Kowal sped up
WeakMaps by using the hiddenRecord directly as the array-like, indexed
by a per-map unique index.

I kept the old defProp that explicitly enumerates the writable,
enumerable, configurable attributes that should default to false
anyway, out of general paranoia about remaining undiagnosed bugs in
Object.defineProperty.

[MarkM, 2014-01-25 04:02:20]

https://codereview.appspot.com/54750043/diff/130003/src/com/google/caja/ses/W...
File src/com/google/caja/ses/WeakMap.js (right):

https://codereview.appspot.com/54750043/diff/130003/src/com/google/caja/ses/W...
src/com/google/caja/ses/WeakMap.js:445: keys.push(key);
On 2014/01/24 23:00:30, kpreid2 wrote:
> If I were feeling especially paranoid (considering such things as
> slow-script-stopping), I might check here the invariant that keys and values
are
> the same length, since a violation would result in mis-associating all future
> keys and values, a particularly horrible outcome.

Thanks for raising it. The possibility had not occurred to me. Rather than
checking, I'm rewriting to make the algorithm more crash safe. Please have a
look.

https://codereview.appspot.com/54750043/diff/130003/src/com/google/caja/ses/W...
src/com/google/caja/ses/WeakMap.js:461: values.splice(index, 1);
On 2014/01/24 23:00:30, kpreid2 wrote:
> Splicing is probably O(n); instead, move the last entry into this index. Since
> WeakMaps are non-enumerable the ordering makes no observable difference.
> 
> (Should you choose to adopt the abovementioned paranoia, erase either the key
or
> value of the destination, copy the other, and copy the blanked value.)

Done.

https://codereview.appspot.com/54750043/diff/130003/tests/com/google/caja/ses...
File tests/com/google/caja/ses/test-ses-parts.js (right):

https://codereview.appspot.com/54750043/diff/130003/tests/com/google/caja/ses...
tests/com/google/caja/ses/test-ses-parts.js:15: var preFrozen =
Object.freeze({});
On 2014/01/24 23:00:30, kpreid2 wrote:
> +1 for thoroughness

Thanks.

https://codereview.appspot.com/54750043/diff/130003/tests/com/google/caja/ses...
tests/com/google/caja/ses/test-ses-parts.js:38: var wm1 = new WeakMap();
On 2014/01/24 23:00:30, kpreid2 wrote:
> Either give these more descriptive names or add comments explaining which
roles
> they play in the test, so that one doesn't have to figure out the logic of the
> tests by reading all the asserts.

Done.

https://codereview.appspot.com/54750043/diff/130003/tests/com/google/caja/ses...
tests/com/google/caja/ses/test-ses-parts.js:43: assertEquals(wm1.set(postFrozen,
10), wm1);
On 2014/01/24 23:00:30, kpreid2 wrote:
> Expected values go first, actual values go second. Jsunit error messages are
> written to assume that.

I didn't know that. Done.

[kpreid2, 2014-01-27 19:48:32]

https://codereview.appspot.com/54750043/diff/150001/src/com/google/caja/ses/W...
File src/com/google/caja/ses/WeakMap.js (right):

https://codereview.appspot.com/54750043/diff/150001/src/com/google/caja/ses/W...
src/com/google/caja/ses/WeakMap.js:452: // If we crash here, values will be one
longer than keys.
Maybe spend a few words to point out that this is true only when this is the
only thing which extends the array.

https://codereview.appspot.com/54750043/diff/150001/src/com/google/caja/ses/W...
src/com/google/caja/ses/WeakMap.js:487: // is still void 0, since the aliasing
killed the previous key.
If it weren't so extensively commented, I would call this code excessively
clever.

The tests we have are for deleting A from key-set [A]. Given the cleverness, it
might be worth having tests for deleting A from [A, B, C] and [B, A] as well.

[Mark S. Miller, 2014-01-27 20:44:02]

CL already submitted and closed per your LGTM. Do you want me to open a
fresh one?


On Mon, Jan 27, 2014 at 11:48 AM, <kpreid.switchb.org@gmail.com> wrote:

>
> https://codereview.appspot.com/54750043/diff/150001/src/
> com/google/caja/ses/WeakMap.js
> File src/com/google/caja/ses/WeakMap.js (right):
>
> https://codereview.appspot.com/54750043/diff/150001/src/
> com/google/caja/ses/WeakMap.js#newcode452
> src/com/google/caja/ses/WeakMap.js:452: // If we crash here, values will
> be one longer than keys.
> Maybe spend a few words to point out that this is true only when this is
> the only thing which extends the array.
>
> https://codereview.appspot.com/54750043/diff/150001/src/
> com/google/caja/ses/WeakMap.js#newcode487
> src/com/google/caja/ses/WeakMap.js:487: // is still void 0, since the
> aliasing killed the previous key.
> If it weren't so extensively commented, I would call this code
> excessively clever.
>
> The tests we have are for deleting A from key-set [A]. Given the
> cleverness, it might be worth having tests for deleting A from [A, B, C]
> and [B, A] as well.
>
>
> https://codereview.appspot.com/54750043/
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "Google Caja Discuss" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to google-caja-discuss+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>



-- 
    Cheers,
    --MarkM

[kpreid2, 2014-01-27 21:17:52]

On 2014/01/27 20:44:02, Mark S. Miller wrote:
> CL already submitted and closed per your LGTM.

You said "Please have a look." in reply, so I had a look.

> Do you want me to open a fresh one?

I would rather you not have committed unreviewed code (note that your changes
were more than just "Done.", which is what I understand a LGTM-plus-comments to
permit), but given what's there now I see no things that really need changing.