Thanks. Traveling and TC39 this week but I think I'll have time tomorrow to update this.
Normally a typo like this would be caught by a unit test. In this case the unit test framework is deficient. I'll update it to add an arguments length check in another CL. On Jun 2, 2014 7:53 AM, <mstarzin...@chromium.org> wrote: > > LGTM from my end. Just a couple of remarks. > > > https://codereview.chromium.org/259883002/diff/160001/src/bootstrapper.cc > File src/bootstrapper.cc (right): > > https://codereview.chromium.org/259883002/diff/160001/src/bootstrapper.cc#newcode2020 > src/bootstrapper.cc:2020: INSTALL_EXPERIMENTAL_NATIVE(i, collections, > "collection-iterator.js") > Just out of curiosity, is there a particular reason for putting this > into a separate file? How would you feel about merging it into the > existing collection.js file? > > https://codereview.chromium.org/259883002/diff/160001/src/runtime.cc > File src/runtime.cc (right): > > https://codereview.chromium.org/259883002/diff/160001/src/runtime.cc#newcode1597 > src/runtime.cc:1597: holder->set_index(Smi::FromInt(0)); > Remark: This gives a short window of time during which "index" and > "kind" are initialized to undefined (which is totally fine). The > JSSetIteratorVerify assumes those two fields are always Smi. In the > current implementation I don't see how anything in between the > allocation of an iterator and here could cause a heap verification to be > triggered. > > I just wanted to leave this comment for posterity. Should there ever be > a change that causes a GC along the way from the constructor to here, > then the verification method needs to be loosened. > > https://codereview.chromium.org/259883002/diff/160001/src/runtime.cc#newcode1701 > src/runtime.cc:1701: holder->set_index(Smi::FromInt(0)); > Likewise. > > https://codereview.chromium.org/259883002/diff/160001/test/mjsunit/harmony/collection-iterator.js > File test/mjsunit/harmony/collection-iterator.js (right): > > https://codereview.chromium.org/259883002/diff/160001/test/mjsunit/harmony/collection-iterator.js#newcode20 > test/mjsunit/harmony/collection-iterator.js:20: assertEquals(new > Set().values().__proto__. SetIteratorPrototype); > typo: there is a period instead of a comma after "__proto__" here. Gotta > love JavaScript for that. :/ > > https://codereview.chromium.org/259883002/diff/160001/test/mjsunit/harmony/collection-iterator.js#newcode115 > test/mjsunit/harmony/collection-iterator.js:115: assertEquals(new > Map().values().__proto__. MapIteratorPrototype); > typo: Likewise. > > > https://codereview.chromium.org/259883002/ > > -- > -- > v8-dev mailing list > v8-dev@googlegroups.com > 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 v8-dev+unsubscr...@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. -- -- v8-dev mailing list v8-dev@googlegroups.com 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 v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
