Closed Bug 883434 Opened 6 years ago Closed 6 years ago

Add a constructor TokenPos::TokenPos(uint32_t, uint32_t)

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(1 file)

Because TokenPos::make looks dumb.
Attached patch v1Splinter Review
Assignee: general → jorendorff
Attachment #762990 - Flags: review?(jwalden+bmo)
Comment on attachment 762990 [details] [diff] [review]
v1

Review of attachment 762990 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/ParseNode.h
@@ +431,5 @@
>  
>    public:
>      ParseNode(ParseNodeKind kind, JSOp op, ParseNodeArity arity)
>        : pn_type(kind), pn_op(op), pn_arity(arity), pn_parens(0), pn_used(0), pn_defn(0),
> +        pn_pos(0, 0), pn_offset(0), pn_next(NULL), pn_link(NULL)

We should try to work toward a world where position is either uninitialized until set, or ideally is set at construction time.  Clearly gonna take awhile to get there, of course.

@@ +893,5 @@
>  {
>      TernaryNode(ParseNodeKind kind, JSOp op, ParseNode *kid1, ParseNode *kid2, ParseNode *kid3)
>        : ParseNode(kind, op, PN_TERNARY,
> +                  TokenPos((kid1 ? kid1 : kid2 ? kid2 : kid3)->pn_pos.begin,
> +                           (kid3 ? kid3 : kid2 ? kid2 : kid1)->pn_pos.end))

Ugh, that's awful.  But I guess pre-existing.  :-\

::: js/src/frontend/SyntaxParseHandler.h
@@ +41,5 @@
>  
>      SyntaxParseHandler(JSContext *cx, TokenStream &tokenStream, bool foldConstants,
>                         Parser<SyntaxParseHandler> *syntaxParser, LazyScript *lazyOuterFunction)
>        : lastAtom(NULL),
> +        lastStringPos(0, 0),

Hmm, why?  It was fine for this to be not initialized before.  Why shouldn't it be fine for the same now?
Attachment #762990 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> >      SyntaxParseHandler(JSContext *cx, TokenStream &tokenStream, bool foldConstants,
> >                         Parser<SyntaxParseHandler> *syntaxParser, LazyScript *lazyOuterFunction)
> >        : lastAtom(NULL),
> > +        lastStringPos(0, 0),
> 
> Hmm, why?  It was fine for this to be not initialized before.  Why shouldn't
> it be fine for the same now?

Ah. Probably due to an early stab at this bug where I tried not providing a default constructor; that didn't get very far though. Removed.
https://hg.mozilla.org/mozilla-central/rev/cb1358a3b514
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.