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

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 639055 [details] [diff] [review]
Part 1 - STMT_IS_TRYING and friends, v1
Assignee: general → jorendorff
Attachment #639055 - Flags: review?(n.nethercote)
(Assignee)

Comment 2

5 years ago
Created attachment 639056 [details] [diff] [review]
Part 2 - Convert StmtInfoBase::flags to bitfields, v1
Attachment #639056 - Flags: review?(n.nethercote)
(Assignee)

Comment 3

5 years ago
Created attachment 639057 [details] [diff] [review]
Part 3 - GOSUBS and friends
Attachment #639057 - Flags: review?(n.nethercote)
(Assignee)

Comment 4

5 years ago
Created attachment 639058 [details] [diff] [review]
Part 4 - move isFunctionBodyBlock from StmtInfoBase to StmtInfoTC, v1

It is only used during parsing, not emitting.
Attachment #639058 - Flags: review?(n.nethercote)
(Assignee)

Updated

5 years ago
Blocks: 770849
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.
Attachment #639055 - Flags: review?(n.nethercote) → review+
Attachment #639056 - Flags: review?(n.nethercote) → review+
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.
Attachment #639057 - Flags: review?(n.nethercote) → review+
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.
Attachment #639058 - Flags: review?(n.nethercote) → review+
Duplicate of this bug: 770067
(Assignee)

Comment 9

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

Comment 10

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

Comment 11

5 years ago
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
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.