770846 - Convert some macros to methods and flags to bitfields in TreeContext.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Part 1 - STMT_IS_TRYING and friends, v1

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Part 2 - Convert StmtInfoBase::flags to bitfields, v1

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Part 3 - GOSUBS and friends

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Part 4 - move isFunctionBodyBlock from StmtInfoBase to StmtInfoTC, v1

It is only used during parsing, not emitting.

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 639055 [details] [diff] [review]
Part 1 - STMT_IS_TRYING and friends, v1

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

Your first three patches are almost identical to my three patches in bug 770067, from three days ago.  I'm happy to abandon my patches in favour of yours because you have follow-ups.  But I'd like to understand how this duplication of work happened -- did you not see my review requests?

::: js/src/frontend/TreeContext.h
@@ +274,5 @@
>  };
>  
>  /*
>   * NB: If you add enumerators for scope statements, add them between STMT_WITH
>   * and STMT_CATCH, or you will break the STMT_TYPE_IS_SCOPE macro. If you add

STMT_TYPE_IS_SCOPE has been renamed.  Also, you changed it from a range test to "type == STMT_WITH || type == STMT_CATCH", so this comment is no longer correct.  I suggest you keep it as a range test;  that's what I did in my patch.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 639057 [details] [diff] [review]
Part 3 - GOSUBS and friends

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

::: js/src/frontend/TreeContext.h
@@ +397,5 @@
> +
> +    ptrdiff_t &gosubs() {
> +        JS_ASSERT(type == STMT_FINALLY);
> +        return breaks;
> +    }

I used unions in my patch, but I didn't have the type assertions.  Maybe you could use unions?  And/or: having protected these accesses with assertions, is it worth protecting the update/breaks/continues accesses similarly?  I'll let you decide;  what you have is already a clear improvement.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 639058 [details] [diff] [review]
Part 4 - move isFunctionBodyBlock from StmtInfoBase to StmtInfoTC, v1

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

Finally, a patch that isn't familiar :P  Nice change.

Comment 9

[Jason Orendorff [:jorendorff]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> Comment on attachment 639055 [details] [diff] [review]
> Part 1 - STMT_IS_TRYING and friends, v1
> 
> Review of attachment 639055 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Your first three patches are almost identical to my three patches in bug
> 770067, from three days ago.  I'm happy to abandon my patches in favour of
> yours because you have follow-ups.  But I'd like to understand how this
> duplication of work happened -- did you not see my review requests?

Right, I just hadn't gotten to them yet. Great minds think alike, I guess.

Thanks for reviewing anyway.

> >   * NB: If you add enumerators for scope statements, add them between STMT_WITH
> >   * and STMT_CATCH, or you will break the STMT_TYPE_IS_SCOPE macro. If you add
> 
> STMT_TYPE_IS_SCOPE has been renamed.  Also, you changed it from a range test
> to "type == STMT_WITH || type == STMT_CATCH", so this comment is no longer
> correct.  I suggest you keep it as a range test;  that's what I did in my
> patch.

OK.

Comment 10

[Jason Orendorff [:jorendorff]]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> ::: js/src/frontend/TreeContext.h
> @@ +397,5 @@
> > +
> > +    ptrdiff_t &gosubs() {
> > +        JS_ASSERT(type == STMT_FINALLY);
> > +        return breaks;
> > +    }
> 
> I used unions in my patch, but I didn't have the type assertions.  Maybe you
> could use unions?  And/or: having protected these accesses with assertions,
> is it worth protecting the update/breaks/continues accesses similarly?  I'll
> let you decide;  what you have is already a clear improvement.

Oh, you know what we should do, we should make a subclass for each of those, and assert when downcasting rather than on every access.

Comment 11

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ef163668331
https://hg.mozilla.org/integration/mozilla-inbound/rev/af6684f3e870
https://hg.mozilla.org/integration/mozilla-inbound/rev/887f1eb6307c
https://hg.mozilla.org/integration/mozilla-inbound/rev/1796f50e5d1a

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2ef163668331
https://hg.mozilla.org/mozilla-central/rev/af6684f3e870
https://hg.mozilla.org/mozilla-central/rev/887f1eb6307c
https://hg.mozilla.org/mozilla-central/rev/1796f50e5d1a