Last Comment Bug 710932 - Create more parse node types from pure constructors
: Create more parse node types from pure constructors
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-14 16:54 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-12-30 04:52 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Create debugger with a constructor (1.21 KB, patch)
2011-12-14 16:55 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Review
Create <?target data?> with a constructor (11.43 KB, patch)
2011-12-14 16:57 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Review
Create ?: nodes with a constructor (8.41 KB, patch)
2011-12-14 17:01 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Review
Create defsharp/usesharp with a constructor (9.27 KB, patch)
2011-12-14 17:03 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Review
Create true/false/null/this from constructors (2.29 KB, patch)
2011-12-14 17:05 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-14 16:54:58 PST
The nullary kinds are the easiest ones to start with, because they have no dependence on the layout of the ParseNode::pn_u's unary/binary/ternary/&c. fields.  (They sometimes/often bootleg particular fields, though, just to keep things interesting.)  This bug will take care of a small handful of the easy ones.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-14 16:55:35 PST
Created attachment 581832 [details] [diff] [review]
Create debugger with a constructor
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-14 16:57:21 PST
Created attachment 581833 [details] [diff] [review]
Create <?target data?> with a constructor

It's pretty awesome how we had this idea that the data field of a PI could be null, despite that not being the case (it's really just the empty string in that case).
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-14 17:01:14 PST
Created attachment 581834 [details] [diff] [review]
Create ?: nodes with a constructor

Not actually a nullary node, but it was what I happened to pick next, before encountering the arity code that precludes truly segregating all accesses to fields through a type-safe class.  This also renames PNK_HOOK to PNK_CONDITIONAL, to follow the spec name of ConditionalExpression better.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-14 17:03:27 PST
Created attachment 581835 [details] [diff] [review]
Create defsharp/usesharp with a constructor

defsharp's nullary and thus easy to convert to a class with all private members, usesharp's unary and so must keep using the shared pn_u.unary member.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-14 17:05:08 PST
Created attachment 581836 [details] [diff] [review]
Create true/false/null/this from constructors

The classes are kind of more for consistency than anything else here, but they do get rid of mention of the JSOp/arity/etc. stuff at the call sites, which will be helpful in the long run.
Comment 6 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-12-14 21:51:05 PST
I give you a gold star for cleaning this stuff up.  In my brief forays in the parser I found the ad hoc initialization of parse node fields terrifying.
Comment 7 :Ms2ger 2011-12-29 05:07:19 PST
Comment on attachment 581834 [details] [diff] [review]
Create ?: nodes with a constructor

>--- a/js/src/frontend/ParseNode.h
>+++ b/js/src/frontend/ParseNode.h
>- * PNK_HOOK     ternary     pn_kid1: cond, pn_kid2: then, pn_kid3: else
>+ * PNK_CONDITIONAL ternary  (cond ? trueExpr : falseExpr)
>+ *                          pn_kid1: cond, pn_kid2: then, pn_kid3: else

I wonder if it makes sense to use the same names in both lines? (Like 'pn_kid1: cond, pn_kid2: trueExpr, pn_kid3: falseExpr')
Comment 8 Jason Orendorff [:jorendorff] 2011-12-29 17:26:20 PST
Comment on attachment 581833 [details] [diff] [review]
Create <?target data?> with a constructor

Is there an underlying rule that explains when you use references and when you use pointers? It seems pretty random.

In any case, don't make a parameter a non-const reference if const would work. I think all of these can be made const with no problems, but if not, as might be the case for JSAtom, use a pointer.

(The main reason is that implicit aliasing with the possibility of mutation is bad. Another reason is that references add another bit for everyone to memorize, per parameter, per function--that's just a nuisance. Another is that while references can't be null in theory, they can be in practice--in fact the compiler doesn't even help ensure that they are non-null by doing any kind of partial proof--yet you can't assert that a reference is non-null. Another reason is that I like & to mean something. When you mix pointers and references, you end up with * and & at meaningless points in the code--I saw a few examples in this patch--where they literally compile to no machine code even with optimizations off. In short, non-const references are a headgun: they punish people who try to reason about code. I can live with the non-const references that are local variables, but the code would be better without them.)

In TokenStream.h, struct Token:
>+            PropertyName *target;       /* non-empty */
>+            JSAtom       *data;         /* potentially empty */

I was confused at first. Recommend "/* may be empty, never null */".

r=me with those changes.
Comment 9 Jason Orendorff [:jorendorff] 2011-12-29 17:36:14 PST
Actually I have to go back on being able to live with some of the references in that patch. I can live with some, but let's focus on JSAtom* and ParseNodes.

Currently almost all addresses of ParseNodes are currently represented as pointers. That is actually a pretty nice property. We should keep it that way (just to avoid the guess-the-signature nuisance, if nothing else) unless there is some serious reason not to.
Comment 10 Jason Orendorff [:jorendorff] 2011-12-29 17:39:16 PST
Comment on attachment 581834 [details] [diff] [review]
Create ?: nodes with a constructor

In struct ParseNode:
>+    void operator=(const ParseNode &other) MOZ_DELETE;

Thanks!

r=me, same comment about the references.
Comment 11 Jason Orendorff [:jorendorff] 2011-12-29 17:48:43 PST
P.S. Gold star co-sign.

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