710932 - Create more parse node types from pure constructors

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

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]]

Attachment: Create debugger with a constructor

Comment 2

[Jeff Walden [:Waldo]]

Attachment: 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]]

Attachment: 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]]

Attachment: 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]]

Attachment: 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 [inactive]]

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 (he/him; ⌚ UTC+1/+2)]

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]]

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]]

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]]

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]]

P.S. Gold star co-sign.

Comment 12

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f5264f18270
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2f87c9d7a71
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1ca7fe3d8e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1560ef4222c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca3ba6070968

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/9f5264f18270
https://hg.mozilla.org/mozilla-central/rev/a2f87c9d7a71
https://hg.mozilla.org/mozilla-central/rev/a1ca7fe3d8e2
https://hg.mozilla.org/mozilla-central/rev/b1560ef4222c
https://hg.mozilla.org/mozilla-central/rev/ca3ba6070968