Last Comment Bug 770846 - Convert some macros to methods and flags to bitfields in TreeContext.h
: Convert some macros to methods and flags to bitfields in TreeContext.h
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jason Orendorff [:jorendorff]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 770067 (view as bug list)
Depends on:
Blocks: 770849
  Show dependency treegraph
 
Reported: 2012-07-04 04:57 PDT by Jason Orendorff [:jorendorff]
Modified: 2012-07-07 12:01 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - STMT_IS_TRYING and friends, v1 (14.18 KB, patch)
2012-07-04 05:00 PDT, Jason Orendorff [:jorendorff]
n.nethercote: review+
Details | Diff | Splinter Review
Part 2 - Convert StmtInfoBase::flags to bitfields, v1 (13.45 KB, patch)
2012-07-04 05:01 PDT, Jason Orendorff [:jorendorff]
n.nethercote: review+
Details | Diff | Splinter Review
Part 3 - GOSUBS and friends (9.07 KB, patch)
2012-07-04 05:04 PDT, Jason Orendorff [:jorendorff]
n.nethercote: review+
Details | Diff | Splinter Review
Part 4 - move isFunctionBodyBlock from StmtInfoBase to StmtInfoTC, v1 (3.43 KB, patch)
2012-07-04 05:22 PDT, Jason Orendorff [:jorendorff]
n.nethercote: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2012-07-04 04:57:21 PDT

    
Comment 1 Jason Orendorff [:jorendorff] 2012-07-04 05:00:16 PDT
Created attachment 639055 [details] [diff] [review]
Part 1 - STMT_IS_TRYING and friends, v1
Comment 2 Jason Orendorff [:jorendorff] 2012-07-04 05:01:31 PDT
Created attachment 639056 [details] [diff] [review]
Part 2 - Convert StmtInfoBase::flags to bitfields, v1
Comment 3 Jason Orendorff [:jorendorff] 2012-07-04 05:04:36 PDT
Created attachment 639057 [details] [diff] [review]
Part 3 - GOSUBS and friends
Comment 4 Jason Orendorff [:jorendorff] 2012-07-04 05:22:27 PDT
Created attachment 639058 [details] [diff] [review]
Part 4 - move isFunctionBodyBlock from StmtInfoBase to StmtInfoTC, v1

It is only used during parsing, not emitting.
Comment 5 Nicholas Nethercote [:njn] 2012-07-04 16:21:47 PDT
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 [:njn] 2012-07-04 16:29:34 PDT
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 [:njn] 2012-07-04 16:30:50 PDT
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 8 Nicholas Nethercote [:njn] 2012-07-04 20:01:23 PDT
*** Bug 770067 has been marked as a duplicate of this bug. ***
Comment 9 Jason Orendorff [:jorendorff] 2012-07-06 10:01:46 PDT
(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] 2012-07-06 10:14:33 PDT
(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.

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