The default bug view has changed. See this FAQ.

Create more parse node types from pure constructors

RESOLVED FIXED in mozilla12



JavaScript Engine
5 years ago
5 years ago


(Reporter: Waldo, Assigned: Waldo)



Firefox Tracking Flags

(Not tracked)



(5 attachments)

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.
Created attachment 581832 [details] [diff] [review]
Create debugger with a constructor
Attachment #581832 - Flags: review?(jorendorff)
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).
Attachment #581833 - Flags: review?(jorendorff)
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.
Attachment #581834 - Flags: review?(jorendorff)
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.
Attachment #581835 - Flags: review?(jorendorff)
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.
Attachment #581836 - Flags: review?(jorendorff)
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 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')
Attachment #581832 - Flags: review?(jorendorff) → review+
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.
Attachment #581833 - Flags: review?(jorendorff) → review+
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 on attachment 581834 [details] [diff] [review]
Create ?: nodes with a constructor

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


r=me, same comment about the references.
Attachment #581834 - Flags: review?(jorendorff) → review+
Attachment #581835 - Flags: review?(jorendorff) → review+
Attachment #581836 - Flags: review?(jorendorff) → review+
P.S. Gold star co-sign.
Target Milestone: --- → mozilla12
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.