[modules] Implement @@iterator on namespace objects.

[modules] Implement @@iterator on namespace objects.

As part of this, introduce a new JSObject for iterating over the elements of a
FixedArray.

R=adamk@chromium.org,bmeurer@chromium.org
TBR=ulan@chromium.org
BUG=v8:1569
Committed: https://crrev.com/dafe6867f33ab95cb113159e9088da905c8de37b
Cr-Commit-Position: refs/heads/master@{#40265}

[neis, 2016-10-11 18:34:00]

Description was changed from

==========
[modules] Implement @@iterator on namespace objects.

As part of this, introduce a new JSObject for iterating over the elements of a
FixedArray.

R=adamk@chromium.org
BUG=v8:1569
==========

to

==========
[modules] Implement @@iterator on namespace objects.

As part of this, introduce a new JSObject for iterating over the elements of a
FixedArray.

R=adamk@chromium.org,bmeurer@chromium.org
BUG=v8:1569
==========

[neis, 2016-10-11 18:36:17]

+bmeurer for the awesomeness in builtins-iterator.cc.

https://codereview.chromium.org/2407423002/diff/1/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2407423002/diff/1/src/bootstrapper.cc#newcode...
src/bootstrapper.cc:2790:
map->SetConstructor(native_context->object_function());
I don't know if I need this line.

https://codereview.chromium.org/2407423002/diff/1/src/builtins/builtins-itera...
File src/builtins/builtins-iterator.cc (right):

https://codereview.chromium.org/2407423002/diff/1/src/builtins/builtins-itera...
src/builtins/builtins-iterator.cc:47: }
This is awesome, isn't it?  Is there any better way to do this?

[neis, 2016-10-11 18:36:18]

+bmeurer for the awesomeness in builtins-iterator.cc.

[adamk, 2016-10-11 19:42:53]

adamk@chromium.org changed reviewers:
+ bmeurer@chromium.org

[adamk, 2016-10-11 19:42:53]

lgtm other than the BuiltinFrame code (which I'm not at all familiar with).

https://codereview.chromium.org/2407423002/diff/1/src/builtins/builtins-itera...
File src/builtins/builtins-iterator.cc (right):

https://codereview.chromium.org/2407423002/diff/1/src/builtins/builtins-itera...
src/builtins/builtins-iterator.cc:47: }
On 2016/10/11 18:36:17, neis wrote:
> This is awesome, isn't it?  Is there any better way to do this?

This looks about right to me (this "next" function should be the same one
attached to the receiver), except for the frame stuff (on which I defer to
bmeurer).

[Benedikt Meurer, 2016-10-12 03:24:06]

Nice! LGTM once comments addressed.

https://codereview.chromium.org/2407423002/diff/1/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2407423002/diff/1/src/bootstrapper.cc#newcode...
src/bootstrapper.cc:2171: // TODO(neis): Is this really supposed to be writable?
You can remove this TODO. All the @@iterator properties are writable (for
whatever reason).

https://codereview.chromium.org/2407423002/diff/1/src/builtins/builtins-itera...
File src/builtins/builtins-iterator.cc (right):

https://codereview.chromium.org/2407423002/diff/1/src/builtins/builtins-itera...
src/builtins/builtins-iterator.cc:35: BUILTIN(FixedArrayIteratorNext) {
Nice idea!

https://codereview.chromium.org/2407423002/diff/1/src/builtins/builtins-itera...
src/builtins/builtins-iterator.cc:47: }
Adam is right, you need this.

But please don't go hunting in the stack frames. There's args.target() which
yields the closure, just use that instead. And please rephrase it as a single
if, without the magical error variable.

https://codereview.chromium.org/2407423002/diff/1/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2407423002/diff/1/src/factory.cc#newcode2617
src/factory.cc:2617:
isolate()->builtins()->builtin(Builtins::kFixedArrayIteratorNext));
This is pretty inefficient, as you create a new SharedFunctionInfo on each
invocation. Please add a TODO here that we might want to revisit this at some
point.

[adamk, 2016-10-12 16:21:47]

https://codereview.chromium.org/2407423002/diff/1/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2407423002/diff/1/src/bootstrapper.cc#newcode...
src/bootstrapper.cc:2171: // TODO(neis): Is this really supposed to be writable?
On 2016/10/12 03:24:05, Benedikt Meurer wrote:
> You can remove this TODO. All the @@iterator properties are writable (for
> whatever reason).

The Module case is somewhat special, as the spec goes to some length to try to
avoid modifications to the Module object; that's what this TODO is about.

[adamk, 2016-10-12 17:29:42]

I've been discussing some spec issues about this particular feature with folks
on IRC (mozilla.org #jslang) and it seems like the oddity of this iterator is a
spec bug: there's no reason we couldn't have a
%ModuleNamespaceIteratorPrototype% to store the next method on, and bterlson
(the editor) is open to adding such a prototype.

[Benedikt Meurer, 2016-10-13 03:32:19]

On 2016/10/12 17:29:42, adamk wrote:
> I've been discussing some spec issues about this particular feature with folks
> on IRC (http://mozilla.org #jslang) and it seems like the oddity of this
iterator is a
> spec bug: there's no reason we couldn't have a
> %ModuleNamespaceIteratorPrototype% to store the next method on, and bterlson
> (the editor) is open to adding such a prototype.

That would be nice. We could already implement it that way, then we don't need
to do the dance in @@iterator.

[neis, 2016-10-13 07:37:21]

https://codereview.chromium.org/2407423002/diff/1/src/builtins/builtins-itera...
File src/builtins/builtins-iterator.cc (right):

https://codereview.chromium.org/2407423002/diff/1/src/builtins/builtins-itera...
src/builtins/builtins-iterator.cc:47: }
On 2016/10/12 03:24:05, Benedikt Meurer wrote:
> Adam is right, you need this.
> 
> But please don't go hunting in the stack frames. There's args.target() which
> yields the closure, just use that instead. And please rephrase it as a single
> if, without the magical error variable.

Thanks for pointing me to args.target(), that's exactly what I was looking for
but couldn't find at the time.

I don't think there was anything magical about the error variable but now I
don't need it anymore.

https://codereview.chromium.org/2407423002/diff/1/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2407423002/diff/1/src/factory.cc#newcode2617
src/factory.cc:2617:
isolate()->builtins()->builtin(Builtins::kFixedArrayIteratorNext));
On 2016/10/12 03:24:05, Benedikt Meurer wrote:
> This is pretty inefficient, as you create a new SharedFunctionInfo on each
> invocation. Please add a TODO here that we might want to revisit this at some
> point.

Right, thanks!

[neis, 2016-10-13 07:37:44]

The CQ bit was checked by neis@chromium.org

[neis, 2016-10-13 07:37:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2407423002/#ps20001
(title: "Address review comments.")

[commit-bot: I haz the power, 2016-10-13 07:37:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 07:43:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-13 07:43:03]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/26335)

[neis, 2016-10-13 11:38:20]

Patchset #3 (id:40001) has been deleted

[neis, 2016-10-13 11:38:28]

Patchset #3 (id:60001) has been deleted

[neis, 2016-10-13 11:38:50]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 11:38:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[neis, 2016-10-13 11:41:04]

It turns out my JSFixedArrayIterator object was broken (e.g. adding a new
property to it resulted in a crash).  Patch Set 4 fixes this.  Benedikt, PTAL.

[neis, 2016-10-13 11:42:29]

neis@chromium.org changed reviewers:
+ ishell@chromium.org

[neis, 2016-10-13 11:42:40]

On 2016/10/13 11:41:04, neis wrote:
> It turns out my JSFixedArrayIterator object was broken (e.g. adding a new
> property to it resulted in a crash).  Patch Set 4 fixes this.  Benedikt, PTAL.

+ishell: you too please :)

[commit-bot: I haz the power, 2016-10-13 11:55:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-13 11:55:07]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Igor Sheludko, 2016-10-13 11:58:27]

lgtm with nits:

https://codereview.chromium.org/2407423002/diff/120001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2407423002/diff/120001/src/objects.h#newcode1...
src/objects.h:10458: DECL_ACCESSORS(next, JSFunction)
Maybe call it initial_next to avoid confusion with the normal "next" property.

https://codereview.chromium.org/2407423002/diff/120001/src/objects.h#newcode1...
src/objects.h:10463: static const int kHeaderSize = kNextOffset + kPointerSize;
Please name it kSize to be consistent.

[Benedikt Meurer, 2016-10-13 12:18:10]

Still LGTM. Magic 1 for the win!

[neis, 2016-10-13 12:18:37]

>
https://codereview.chromium.org/2407423002/diff/120001/src/objects.h#newcode1...
> src/objects.h:10458: DECL_ACCESSORS(next, JSFunction)
> Maybe call it initial_next to avoid confusion with the normal "next" property.

Ack.

>
https://codereview.chromium.org/2407423002/diff/120001/src/objects.h#newcode1...
> src/objects.h:10463: static const int kHeaderSize = kNextOffset +
kPointerSize;
> Please name it kSize to be consistent.

Some objects do define kHeaderSize instead of kSize and I thought kHeaderSize is
more accurate in this case. What is the rule here?

[neis, 2016-10-13 12:29:25]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 12:29:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Igor Sheludko, 2016-10-13 12:47:37]

On 2016/10/13 12:18:37, neis wrote:
> Some objects do define kHeaderSize instead of kSize and I thought kHeaderSize
is
> more accurate in this case. What is the rule here?

Indeed. Then I'm fine with kHeaderSize.

[neis, 2016-10-13 12:55:23]

Description was changed from

==========
[modules] Implement @@iterator on namespace objects.

As part of this, introduce a new JSObject for iterating over the elements of a
FixedArray.

R=adamk@chromium.org,bmeurer@chromium.org
BUG=v8:1569
==========

to

==========
[modules] Implement @@iterator on namespace objects.

As part of this, introduce a new JSObject for iterating over the elements of a
FixedArray.

R=adamk@chromium.org,bmeurer@chromium.org
TBR=ulan@chromium.org
BUG=v8:1569
==========

[neis, 2016-10-13 12:56:55]

The CQ bit was checked by neis@chromium.org

[neis, 2016-10-13 12:56:55]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
bmeurer@chromium.org, ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2407423002/#ps180001
(title: "Rename kSize to kHeadersize again.")

[commit-bot: I haz the power, 2016-10-13 12:57:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 13:34:57]

Description was changed from

==========
[modules] Implement @@iterator on namespace objects.

As part of this, introduce a new JSObject for iterating over the elements of a
FixedArray.

R=adamk@chromium.org,bmeurer@chromium.org
TBR=ulan@chromium.org
BUG=v8:1569
==========

to

==========
[modules] Implement @@iterator on namespace objects.

As part of this, introduce a new JSObject for iterating over the elements of a
FixedArray.

R=adamk@chromium.org,bmeurer@chromium.org
TBR=ulan@chromium.org
BUG=v8:1569
==========

[commit-bot: I haz the power, 2016-10-13 13:34:58]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2016-10-13 13:35:11]

Description was changed from

==========
[modules] Implement @@iterator on namespace objects.

As part of this, introduce a new JSObject for iterating over the elements of a
FixedArray.

R=adamk@chromium.org,bmeurer@chromium.org
TBR=ulan@chromium.org
BUG=v8:1569
==========

to

==========
[modules] Implement @@iterator on namespace objects.

As part of this, introduce a new JSObject for iterating over the elements of a
FixedArray.

R=adamk@chromium.org,bmeurer@chromium.org
TBR=ulan@chromium.org
BUG=v8:1569

Committed: https://crrev.com/dafe6867f33ab95cb113159e9088da905c8de37b
Cr-Commit-Position: refs/heads/master@{#40265}
==========

[commit-bot: I haz the power, 2016-10-13 13:35:12]

Patchset 8 (id:??) landed as
https://crrev.com/dafe6867f33ab95cb113159e9088da905c8de37b
Cr-Commit-Position: refs/heads/master@{#40265}