create a nicer C++ for "for of" iteration

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Benjamin, Unassigned)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
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.
Attachment #630791 - Flags: review?(jorendorff)

Comment 2

5 years ago
// ForIterator it(cx, iteratable)

This should be ForOfIterator.
(Reporter)

Comment 3

5 years ago
Created attachment 631022 [details] [diff] [review]
v2 - a few tweaks
Attachment #630791 - Attachment is obsolete: true
Attachment #630791 - Flags: review?(jorendorff)
Attachment #631022 - Flags: review?(jorendorff)
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.
Attachment #631022 - Flags: review?(jorendorff)
Created attachment 631208 [details] [diff] [review]
v3 - slightly different API
Attachment #631208 - Flags: review?(bpeterson)
(Reporter)

Comment 6

5 years ago
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.
Attachment #631208 - Flags: review?(bpeterson) → review+
(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).
(Reporter)

Comment 8

5 years ago
(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?
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.
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.
(Reporter)

Updated

5 years ago
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/ee249716d49d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.