695752 - Give js::ParseNode a constuctor and make its subclasses new_-able

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

One more step down the road toward parser utopia:

>-    ParseNode *pn = NewOrRecycledNode(tc);
>-    if (!pn)
>-        return NULL;
>-    pn->init(tt, op, PN_BINARY);
>-    pn->pn_pos.begin = left->pn_pos.begin;
>-    pn->pn_pos.end = right->pn_pos.end;
>-    pn->pn_left = left;
>-    pn->pn_right = right;
>-    return pn;
>+    return tc->parser->new_<BinaryNode>(tt, op, left, right);

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Part 1 - Give js::ParseNode a constructor

This gives class ParseNode a constructor. But it isn't always called. Part 2 fixes that.

Even without part 2, this is OK. NewOrRecycledNode is the place where ParseNodes are created without the constructor being called. It zeroes out most of the struct rather than calling a constructor, and its callers fill in the remaining fields.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Part 2 - Add new_ methods for parse nodes to class Parser - v1

Comment 3

[Jason Orendorff [:jorendorff]]

Part 2 adds a constructor to struct TokenPos, just for convenience. Since the default constructor still does nothing, there's no reason to expect a slowdown from that, right? And the struct is still considered POD, right?

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 568140 [details] [diff] [review]
Part 2 - Add new_ methods for parse nodes to class Parser - v1

>diff --git a/js/src/frontend/ParseNode.cpp b/js/src/frontend/ParseNode.cpp
>+namespace js {

Another one of these styles-in-flux-but-converging is whether to wrap .cpp's in namespace js {} or prefix with js.  I started in jstracer.cpp by wrapping the whole thing in namespace js.  methodjit/ always prefixes, and I think this is the better choice for several reasons so I (and others and any patch I review) have been going that direction (jstracer.cpp is doomed so I'm leaving it be).  Could we do this for frontend/ also?  (I'm sorry if I missed this in previous frontend/ reviews.)  Note: member function definitions don't need a prefix (since there is first a name lookup on the class name which sees the 'using namespace js'), so its only non-static external functions which, if we modularize correctly, shouldn't be too numerous.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Part 3 - class ParseNodeAllocator, v1

Refactor ParseNode allocation, add new_ methods and use them in a few places.

Most places that create parse nodes still call UnaryNode::create(tc) or NameNode::create(tc), etc. They'll just stay that way for now.

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to Luke Wagner [:luke] from comment #4)
> Another one of these styles-in-flux-but-converging is whether to wrap .cpp's
> in namespace js {} or prefix with js.  I started in jstracer.cpp by wrapping
> the whole thing in namespace js.  methodjit/ always prefixes, and I think
> this is the better choice for several reasons

I strongly prefer prefixing, so I'll be happy to fix this. I'll do it in a separate patch if you don't mind.

Comment 7

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a5a2d0857bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e5a4585d465
https://hg.mozilla.org/integration/mozilla-inbound/rev/477f20a61cba

Comment 8

[Jason Orendorff [:jorendorff]]

Follow-up to fix Maemo:

https://hg.mozilla.org/integration/mozilla-inbound/rev/68dc33251642

Comment 9

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/7a5a2d0857bd
https://hg.mozilla.org/mozilla-central/rev/7e5a4585d465
https://hg.mozilla.org/mozilla-central/rev/477f20a61cba
https://hg.mozilla.org/mozilla-central/rev/68dc33251642

please check tree-management since this may have caused a V8 regression