Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Create more parse node types from pure constructors

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(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;

Thanks!

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.
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
Target Milestone: --- → mozilla12
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.