Bug 160800 - Add a helper function for enumerating elements in an iterable object
Summary: Add a helper function for enumerating elements in an iterable object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 160801
  Show dependency treegraph
 
Reported: 2016-08-12 00:11 PDT by Ryosuke Niwa
Modified: 2016-08-12 19:03 PDT (History)
10 users (show)

See Also:


Attachments
Work in progress (4.12 KB, patch)
2016-08-12 00:12 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (13.92 KB, patch)
2016-08-12 16:40 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Functor approach (13.60 KB, patch)
2016-08-12 17:58 PDT, Ryosuke Niwa
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2016-08-12 00:11:31 PDT
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.
Comment 1 Ryosuke Niwa 2016-08-12 00:12:09 PDT
Created attachment 285900 [details]
Work in progress

Here's my working progress patch. Do you think a class like this is useful / desirable?
Comment 2 Saam Barati 2016-08-12 11:48:27 PDT
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.
Comment 3 Ryosuke Niwa 2016-08-12 12:28:24 PDT
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.
Comment 4 Ryosuke Niwa 2016-08-12 12:33:35 PDT
Basically what I want to create is https://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization for going over an iterable.
Comment 5 Filip Pizlo 2016-08-12 12:56:36 PDT
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.
Comment 6 Saam Barati 2016-08-12 15:08:00 PDT
(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.
Comment 7 Saam Barati 2016-08-12 15:11:59 PDT
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
};
Comment 8 Ryosuke Niwa 2016-08-12 16:40:15 PDT
Created attachment 285976 [details]
Patch
Comment 9 Saam Barati 2016-08-12 16:47:50 PDT
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.
Comment 10 Ryosuke Niwa 2016-08-12 17:58:07 PDT
Created attachment 285987 [details]
Functor approach
Comment 11 Benjamin Poulain 2016-08-12 18:10:59 PDT
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?
Comment 12 Ryosuke Niwa 2016-08-12 18:22:15 PDT
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.
Comment 13 Ryosuke Niwa 2016-08-12 19:03:35 PDT
Committed r204438: <http://trac.webkit.org/changeset/204438>