Thanks Arv, looks good so far. Besides the refactoring you suggest I'd
mainly
like to have a few additional tests (see below).
https://codereview.chromium.org/384963002/diff/1/src/contexts.cc
File src/contexts.cc (right):
https://codereview.chromium.org/384963002/diff/1/src/contexts.cc#newcode80
src/contexts.cc:80: while (true) {
On 2014/07/10 23:13:11, arv wrote:
I'll switch to use the new PrototypeIterator.
Yes, that would be better.
https://codereview.chromium.org/384963002/diff/20001/src/objects-inl.h
File src/objects-inl.h (right):
https://codereview.chromium.org/384963002/diff/20001/src/objects-inl.h#newcode1149
src/objects-inl.h:1149: MaybeHandle<Object>
Object::GetPropertyWithReceiver(Handle<JSReceiver> holder,
I'd prefer to inline this function, it doesn't seem worth polluting the
object API with.
https://codereview.chromium.org/384963002/diff/20001/test/mjsunit/harmony/unscopables.js
File test/mjsunit/harmony/unscopables.js (right):
https://codereview.chromium.org/384963002/diff/20001/test/mjsunit/harmony/unscopables.js#newcode8
test/mjsunit/harmony/unscopables.js:8: (function TestBasics() {
Some of these tests should probably be run on different kinds of objects
(normal, array, function, the global, etc.). And in particular,
proxies... ;) (that one maybe in a separate file, to allow separate
shipping).
Also, include a test that not just reads but writes some variables, as
that can take quite different code paths. Likewise, a test where the
properties in question (on the with'ed object) are accessors.
https://codereview.chromium.org/384963002/diff/20001/test/mjsunit/harmony/unscopables.js#newcode103
test/mjsunit/harmony/unscopables.js:103: (function
TestChangeDuringWith() {
It would be good to have a variation of this test where the _same_
variable reference is evaluated several times (e.g. using a loop) with
different results. Just to ensure that no overoptimistic optimisation
can screw anything up.
https://codereview.chromium.org/384963002/diff/20001/test/mjsunit/harmony/unscopables.js#newcode134
test/mjsunit/harmony/unscopables.js:134: (function
TestAccessorReceiver() {
Hm, this test doesn't use unscopables at all...
https://codereview.chromium.org/384963002/diff/20001/test/mjsunit/harmony/unscopables.js#newcode176
test/mjsunit/harmony/unscopables.js:176: Object.defineProperty(object,
Symbol.unscopables, {
It would definitely be good to have variants of the following two tests
using assignments to x. I'm not even sure where those assignment are
supposed to end up... :)
https://codereview.chromium.org/384963002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
