Bug 160800 – Add a helper function for enumerating elements in an iterable object

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Ryosuke Niwa]

I want to improve the support for sequence<T> in WebIDL (http://heycam.github.io/webidl/#create-sequence-from-iterable),
and that requires me to enumerate items in an iterable object in WebCore.

I could manually call iteratorStep, iterhatorClose, but that seemed rather error prone for WebCore hackers.
So I'm thinking of adding a new helper class that nicely wraps it around.

[Ryosuke Niwa]

Created attachment 285900 [details]
Work in progress

Here's my working progress patch. Do you think a class like this is useful / desirable?

[Saam Barati]

Comment on attachment 285900 [details]
Work in progress

View in context: https://bugs.webkit.org/attachment.cgi?id=285900&action=review

> Source/JavaScriptCore/runtime/SetConstructor.cpp:59
> +    JSIteratorEnumerator(ExecState& state, JSValue iterable)

I think ExecState& instead of ExecState* is an anti pattern in JSC

> Source/JavaScriptCore/runtime/SetConstructor.cpp:140
>          if (exec->hadException())
>              return JSValue::encode(jsUndefined());

Is it intentional to call iteratorClose here now? I suspect we don't want that behavior implicit in the destructor.

> Source/JavaScriptCore/runtime/SetConstructor.cpp:148
> +        if (exec->hadException())

IMO, it's really hard to see the correctness of this now because
the iteratorClose happens when JSIteratorEnumerator is destructed.
I think it might be nice to optionally provide a close() method
on the iterator. I'm not sure if it's worth both having the iteratorClose
step implicitly be called. For example, the above is already different behavior than before.

[Ryosuke Niwa]

Comment on attachment 285900 [details]
Work in progress

View in context: https://bugs.webkit.org/attachment.cgi?id=285900&action=review

>> Source/JavaScriptCore/runtime/SetConstructor.cpp:140
>>              return JSValue::encode(jsUndefined());
> 
> Is it intentional to call iteratorClose here now? I suspect we don't want that behavior implicit in the destructor.

Oh, that's just a bug.

The whole point of this class is to have that behavior implicitly so that the user of this class doesn't need to worry about when & when not to call iteratorClose.

[Ryosuke Niwa]

Basically what I want to create is https://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization for going over an iterable.

[Filip Pizlo]

Comment on attachment 285900 [details]
Work in progress

Have you considered using a functor callback thingy instead?

template<typename Func>
void forEach(Exectate* exec, JSValue iterable, const Func& func)
{
     ... do the loop and call func for each element
}

These are usually easier to maintain.

[Saam Barati]

(In reply to comment #4)
> Basically what I want to create is
> https://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization for
> going over an iterable.

Right. I understand your intention. I'm just commenting
that doing that might lead to bugs, because it will cause
exception handling to always handle not calling iteratorClose.
Or, it will require exception handling *to always* call iteratorClose.
Like in the Set code you changed, it depends on where the exception
is thrown. Anyways, I think the API needs to be able to handle
both such cases if we do go with an RAII style.

FWIW, I like Filip's idea.

[Saam Barati]

I guess the reason I don't like the RAII here
is that it just feels a bit wrong for an RAII
destructor to call arbitrary JS code. I'm not sure
we have a pattern like that anywhere else. Maybe
we do. But it seems like it will be error prone.
Especially because of the prevalence of early
returns for exception checks.
What if we do Fil's idea, and the
<template Func> can take arguments:
Func(ExecState*, JSValue value) -> IteratorNextAction

where IteratorNextAction is
enum class IteratorNextAction {
    Next,
    Close,
    etc... as needed
};

[Ryosuke Niwa]

Created attachment 285976 [details]
Patch

[Saam Barati]

Comment on attachment 285976 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285976&action=review

I still like the functor approach. If we go with this way, just one comment on not keeping ExecState& as a field.

> Source/JavaScriptCore/runtime/IteratorOperations.cpp:188
> +        return JSValue(jsUndefined());

This should just be jsUndefined().

> Source/JavaScriptCore/runtime/IteratorOperations.cpp:193
> +        return JSValue(jsUndefined());

ditto

> Source/JavaScriptCore/runtime/IteratorOperations.h:57
> +    ExecState& m_state;

It's really dangerous for ExecState to be held as a field of an object.
This will lead to bugs resulting from someone keeping a JSIteratorEnumerator around
even though the ExecState* it was passed in is no longer valid. Lets just make
it accepts an ExecState* for its next() function.

[Ryosuke Niwa]

Created attachment 285987 [details]
Functor approach

[Benjamin Poulain]

Comment on attachment 285987 [details]
Functor approach

View in context: https://bugs.webkit.org/attachment.cgi?id=285987&action=review

LGTM.

> Source/JavaScriptCore/runtime/IteratorOperations.h:54
> +        if (state->hadException() || next.isFalse())

I suggest getting the VM& in a variable outside the loop and checking the exception on it instead of Exec.
ExecState::hadException() needs load the VM from memory before it can check the exception.

next.isFalse() is probably the more common case and should go first in the branch.

> Source/JavaScriptCore/runtime/IteratorOperations.h:61
> +        callback(state, nextValue);

And maybe give the VM as argument too to be able to check exceptions efficiently?

[Ryosuke Niwa]

Comment on attachment 285987 [details]
Functor approach

View in context: https://bugs.webkit.org/attachment.cgi?id=285987&action=review

>> Source/JavaScriptCore/runtime/IteratorOperations.h:54
>> +        if (state->hadException() || next.isFalse())
> 
> I suggest getting the VM& in a variable outside the loop and checking the exception on it instead of Exec.
> ExecState::hadException() needs load the VM from memory before it can check the exception.
> 
> next.isFalse() is probably the more common case and should go first in the branch.

Will do.

>> Source/JavaScriptCore/runtime/IteratorOperations.h:61
>> +        callback(state, nextValue);
> 
> And maybe give the VM as argument too to be able to check exceptions efficiently?

Will do.

[Ryosuke Niwa]

Committed r204438: <http://trac.webkit.org/changeset/204438>