Last Comment Bug 762285 - create a nicer C++ for "for of" iteration
: create a nicer C++ for "for of" iteration
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla16
Assigned To: general
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-06 15:41 PDT by :Benjamin Peterson
Modified: 2012-06-12 03:05 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
create ForOfIterator (9.03 KB, patch)
2012-06-06 17:52 PDT, :Benjamin Peterson
no flags Details | Diff | Review
v2 - a few tweaks (10.26 KB, patch)
2012-06-07 10:02 PDT, :Benjamin Peterson
no flags Details | Diff | Review
v3 - slightly different API (10.00 KB, patch)
2012-06-07 16:26 PDT, Jason Orendorff [:jorendorff]
benjamin: review+
Details | Diff | Review

Description :Benjamin Peterson 2012-06-06 15:41:00 PDT
There is is currently the js::ForOf template. It requires the boilerplate of creating a class (with a copy constructor). It would be nicer to have a ForOfIterator class, which worked like iterators we have for C++ data structures.
Comment 1 :Benjamin Peterson 2012-06-06 17:52:48 PDT
Created attachment 630791 [details] [diff] [review]
create ForOfIterator

This patch adds a ForOfIterator class. It's nicer than js::ForOf in that it's more general, but there's a bit of unpleasantness from the fact that the iterator must be closed explicitly.
Comment 2 Reece H. Dunn 2012-06-07 03:38:50 PDT
// ForIterator it(cx, iteratable)

This should be ForOfIterator.
Comment 3 :Benjamin Peterson 2012-06-07 10:02:55 PDT
Created attachment 631022 [details] [diff] [review]
v2 - a few tweaks
Comment 4 Jason Orendorff [:jorendorff] 2012-06-07 12:36:06 PDT
Comment on attachment 631022 [details] [diff] [review]
v2 - a few tweaks

Hmm. I'm not crazy about the C++ API presented by ForOfIterator. Let me try something different and I'll have you review it.
Comment 5 Jason Orendorff [:jorendorff] 2012-06-07 16:26:03 PDT
Created attachment 631208 [details] [diff] [review]
v3 - slightly different API
Comment 6 :Benjamin Peterson 2012-06-07 17:05:38 PDT
Comment on attachment 631208 [details] [diff] [review]
v3 - slightly different API

Review of attachment 631208 [details] [diff] [review]:
-----------------------------------------------------------------

The API looks nice. I just have nits.

::: js/src/jsiter.h
@@ +225,5 @@
>   *
> + * The final it.close() check is needed in order to check for cases where
> + * either the constructor or it.next() failed. If anything else fails, as in
> + * this example where DoStuff is fallible, then the call to it.close() may be
> + * skipped. The iterator is automatically cleaned up by ForOfIterator's

This isn't true. it.close() must be called explicitly and checked for error unless an error of some sort is being propagated because CloseIterator is allow raise an exception if there isn't already one.

@@ +243,5 @@
> +  public:
> +    ForOfIterator(JSContext *cx, const Value &iterable)
> +        : cx(cx), iterator(cx, NULL), currentValue(cx), closed(false)
> +    {
> +        RootedValue iterv(cx, iterable);

I see you're now rooting this Value unlike js::ForOf. Was the old code incorrect, is this just a safety measure?

@@ +245,5 @@
> +        : cx(cx), iterator(cx, NULL), currentValue(cx), closed(false)
> +    {
> +        RootedValue iterv(cx, iterable);
> +        ok = ValueToIterator(cx, JSITER_FOR_OF, iterv.address());
> +        iterator = ok ? &iterv.reference().toObject() : NULL;

|iterator| is already initialized to NULL. So, setting it to iterv.toObject() could just be under |if (ok)|.

@@ +250,5 @@
>      }
>  
> +    ~ForOfIterator() {
> +        if (!closed)
> +            close();

Could we assert that this is an appropriate case to let the destructor call close()? That is, a pending exception or OOM.
Comment 7 Jason Orendorff [:jorendorff] 2012-06-08 12:15:57 PDT
(In reply to Benjamin Peterson from comment #6)
> > + * The final it.close() check is needed in order to check for cases where
> > + * either the constructor or it.next() failed. If anything else fails, as in
> > + * this example where DoStuff is fallible, then the call to it.close() may be
> > + * skipped. The iterator is automatically cleaned up by ForOfIterator's
> 
> This isn't true. it.close() must be called explicitly and checked for error
> unless an error of some sort is being propagated because CloseIterator is
> allow raise an exception if there isn't already one.

Right. Fixed. Now it says:

 * The final it.close() check is needed in order to check for cases where
 * any of the iterator operations fail.
 *
 * it.close() may be skipped only if something in the body of the loop fails
 * and the failure is allowed to propagate on cx, as in this example if DoStuff
 * fails. In that case, ForOfIterator's destructor does all necessary cleanup.

> @@ +243,5 @@
> > +  public:
> > +    ForOfIterator(JSContext *cx, const Value &iterable)
> > +        : cx(cx), iterator(cx, NULL), currentValue(cx), closed(false)
> > +    {
> > +        RootedValue iterv(cx, iterable);
> 
> I see you're now rooting this Value unlike js::ForOf. Was the old code
> incorrect, is this just a safety measure?

Well, neither, yet. Currently we use a GC that does conservative stack scanning. So the old code was correct.

But we will be moving to an exact GC, which will require this kind of rooting.
Since the rooting API is available now, it's a good idea to start using it.

> > +        : cx(cx), iterator(cx, NULL), currentValue(cx), closed(false)
> > +    {
> > +        RootedValue iterv(cx, iterable);
> > +        ok = ValueToIterator(cx, JSITER_FOR_OF, iterv.address());
> > +        iterator = ok ? &iterv.reference().toObject() : NULL;
> 
> |iterator| is already initialized to NULL. So, setting it to
> iterv.toObject() could just be under |if (ok)|.

I reset it to NULL to avoid relying on ValueToIterator leaving the outparams untouched on error.

We don't really have strong habits of making sure that functions with outparams provide that as part of the contract, so it's best to code defensively.  :-P
Of course usually it doesn't matter either way; this is a special case.

> @@ +250,5 @@
> > +    ~ForOfIterator() {
> > +        if (!closed)
> > +            close();
> 
> Could we assert that this is an appropriate case to let the destructor call
> close()? That is, a pending exception or OOM.

I don't think there's a way to check for uncatchable errors (OOM and script timeout).
Comment 8 :Benjamin Peterson 2012-06-08 13:35:01 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #7)
> (In reply to Benjamin Peterson from comment #6)
> > > + * The final it.close() check is needed in order to check for cases where
> > > + * either the constructor or it.next() failed. If anything else fails, as in
> > > + * this example where DoStuff is fallible, then the call to it.close() may be
> > > + * skipped. The iterator is automatically cleaned up by ForOfIterator's
> > 
> > This isn't true. it.close() must be called explicitly and checked for error
> > unless an error of some sort is being propagated because CloseIterator is
> > allow raise an exception if there isn't already one.
> 
> Right. Fixed. Now it says:
> 
>  * The final it.close() check is needed in order to check for cases where
>  * any of the iterator operations fail.
>  *
>  * it.close() may be skipped only if something in the body of the loop fails
>  * and the failure is allowed to propagate on cx, as in this example if
> DoStuff
>  * fails. In that case, ForOfIterator's destructor does all necessary
> cleanup.

LGTM

> > Could we assert that this is an appropriate case to let the destructor call
> > close()? That is, a pending exception or OOM.
> 
> I don't think there's a way to check for uncatchable errors (OOM and script
> timeout).

rt->hadOutOfMemory and rt->interrpt?
Comment 9 Jason Orendorff [:jorendorff] 2012-06-11 14:45:27 PDT
rt->interrupt doesn't indicate that any error occurred.

I don't think this can be done without changing the API (which might be a good thing to do, in a separate bug).

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee249716d49d

I accidentally landed this with Benjamin as author and reviewer. Actually I reviewed the parts Benjamin wrote, and he reviewed the parts I wrote.
Comment 10 Jason Orendorff [:jorendorff] 2012-06-11 14:48:16 PDT
rt->hadOutOfMemory is imprecise too: it never gets cleared once it's set. That'd be good enough for an assertion if it weren't for script timeout.
Comment 11 Graeme McCutcheon [:graememcc] 2012-06-12 03:05:11 PDT
https://hg.mozilla.org/mozilla-central/rev/ee249716d49d

Note You need to log in before you can comment on or make changes to this bug.