Bug 160800

Created attachment 285900 [details] Work in progress Here's my working progress patch. Do you think a class like this is useful / desirable?

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 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.

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

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.

(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.

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 };

Created attachment 285976 [details] Patch

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.

Created attachment 285987 [details] Functor approach

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 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.

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