The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
mozilla10
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

6 years ago
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);
(Assignee)

Comment 1

6 years ago
Created attachment 568139 [details] [diff] [review]
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.
Assignee: general → jorendorff
Attachment #568139 - Flags: review?(luke)
(Assignee)

Comment 2

6 years ago
Created attachment 568140 [details] [diff] [review]
Part 2 - Add new_ methods for parse nodes to class Parser - v1
Attachment #568140 - Flags: review?(luke)
(Assignee)

Comment 3

6 years ago
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?

Updated

6 years ago
Attachment #568139 - Flags: review?(luke) → review+

Comment 4

6 years ago
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.
Attachment #568140 - Flags: review?(luke) → review+
(Assignee)

Comment 5

6 years ago
Created attachment 568160 [details] [diff] [review]
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.
Attachment #568160 - Flags: review?(luke)
(Assignee)

Updated

6 years ago
Attachment #568160 - Attachment description: Part 3 - class ParseNodeAllocator and Parser::new_ methods, v1 → Part 3 - class ParseNodeAllocator, v1
(Assignee)

Comment 6

6 years ago
(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.

Updated

6 years ago
Attachment #568160 - Flags: review?(luke) → review+
(Assignee)

Comment 7

6 years ago
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
(Assignee)

Comment 8

6 years ago
Follow-up to fix Maemo:

https://hg.mozilla.org/integration/mozilla-inbound/rev/68dc33251642
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Depends on: 696268
You need to log in before you can comment on or make changes to this bug.