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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Because TokenPos::make looks dumb.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 2

[Jeff Walden [:Waldo]]

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?

Comment 3

[Jason Orendorff [:jorendorff]]

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

Comment 4

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb1358a3b514

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/cb1358a3b514