Last Comment Bug 695752 - Give js::ParseNode a constuctor and make its subclasses new_-able
: Give js::ParseNode a constuctor and make its subclasses new_-able
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
Depends on: 696268
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-19 10:58 PDT by Jason Orendorff [:jorendorff]
Modified: 2011-10-20 19:57 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Give js::ParseNode a constructor (12.01 KB, patch)
2011-10-19 11:48 PDT, Jason Orendorff [:jorendorff]
luke: review+
Details | Diff | Splinter Review
Part 2 - Add new_ methods for parse nodes to class Parser - v1 (20.93 KB, patch)
2011-10-19 11:49 PDT, Jason Orendorff [:jorendorff]
luke: review+
Details | Diff | Splinter Review
Part 3 - class ParseNodeAllocator, v1 (29.23 KB, patch)
2011-10-19 12:55 PDT, Jason Orendorff [:jorendorff]
luke: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2011-10-19 10:58:07 PDT
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] 2011-10-19 11:48:01 PDT
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.
Comment 2 Jason Orendorff [:jorendorff] 2011-10-19 11:49:23 PDT
Created attachment 568140 [details] [diff] [review]
Part 2 - Add new_ methods for parse nodes to class Parser - v1
Comment 3 Jason Orendorff [:jorendorff] 2011-10-19 11:53:31 PDT
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] 2011-10-19 12:12:26 PDT
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] 2011-10-19 12:55:53 PDT
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.
Comment 6 Jason Orendorff [:jorendorff] 2011-10-19 13:24:29 PDT
(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 8 Jason Orendorff [:jorendorff] 2011-10-19 16:38:31 PDT
Follow-up to fix Maemo:

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

Note You need to log in before you can comment on or make changes to this bug.