Last Comment Bug 696941 - Privatize more stuff in Token
: Privatize more stuff in Token
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2011-10-24 16:13 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-10-25 08:18 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Privatize reflags (5.94 KB, patch)
2011-10-24 16:13 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
cdleary: review+
Details | Diff | Splinter Review
Privatize dval (5.29 KB, patch)
2011-10-24 16:15 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
cdleary: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-24 16:13:10 PDT
Created attachment 569215 [details] [diff] [review]
Privatize reflags

Further steps to being able to understand tokens, and to make translating them into parse nodes simpler...
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-24 16:15:17 PDT
Created attachment 569217 [details] [diff] [review]
Privatize dval

This is somewhat more than "just" privatizing, because I split dval into a uint16 used only for referring to sharp variables and a number version used for everything else.  This theoretically introduces more chances for mis-punning, but the uses are all pretty semantically guarded against error, and it clarifies things a bit, so it seems the right thing to do.
Comment 2 Chris Leary [:cdleary] (not checking bugmail) 2011-10-24 18:51:10 PDT
Comment on attachment 569215 [details] [diff] [review]
Privatize reflags

Review of attachment 569215 [details] [diff] [review]:

::: js/src/frontend/Parser.cpp
@@ +7637,4 @@
>          const jschar *chars = tokenStream.getTokenbuf().begin();
>          size_t length = tokenStream.getTokenbuf().length();
> +        RegExpFlag flags = RegExpFlag(tokenStream.currentToken().regExpFlags());

Coersion is no longer necessary, since it's of the right type.

::: js/src/frontend/TokenStream.h
@@ +300,5 @@
> +        /*
> +         * FIXME: Initialize the rest of this early enough to be able to assert
> +         *        type-safety.
> +         */
> +        JS_ASSERT((flags & ~AllFlags) == 0);

I think the positive assertion |(flags & AllFlags) == flags| reads better, no?
Comment 3 Chris Leary [:cdleary] (not checking bugmail) 2011-10-24 18:54:16 PDT
Comment on attachment 569217 [details] [diff] [review]
Privatize dval

Review of attachment 569217 [details] [diff] [review]:

::: js/src/frontend/TokenStream.h
@@ +306,5 @@
>          u.reflags = flags;
>      }
> +    void setSharpNumber(uint16 sharpNum) {
> +        u.sharpNumber = sharpNum;

Can these assert the same conditions as the getters?
Comment 4 Chris Leary [:cdleary] (not checking bugmail) 2011-10-24 18:54:44 PDT
On second thought, the FIXME in the first patch should probably have a bug number associated with it. r=me with that!
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-24 19:47:45 PDT

Filed bug 697000 (=== 1000 mod 0!) for the bug to be noted in a comment.  Really, that problem applies to every mutator, what with the way we have the one out: through which tp->type is set.  So I added the comment at the top of the mutators section.

That's also why setSharpNumber and such can't assert the same condition as its getter, in case it hadn't become obvious after reading the previous paragraph.
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2011-10-25 03:12:39 PDT
=== 0 mod 1000, you mean?
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-25 08:18:39 PDT
Er.  Yes.  :-)

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