554102 - Cleanup: switch from global TokenStream functions to methods

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Switch to TokenStream methods.

Since getToken and friends have become methods of tokenstream, we should invoke them accordingly.

Comment 1

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Switch to TokenStream methods.

Preserves documentation from the removed functions on the methods.

Comment 2

[Brendan Eich [:brendan]]

Would some inline forwarding methods on JSCompiler (e.g. JSCompiler::getToken) to JSCompiler::tokenStream not be a good reduction in source noise/redundancy?

/be

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #2)

If you'd like -- I don't think it's too noisy to say |tokenStream.*|, but I'm pro-explicit-self, so I'm warped to begin with.

By the by, the pump-primed token stream I'm layering on top of the current token stream has a slightly simplified API:

|prime(flags)| Invoke at the end of productions for priming.
|consume(assertFlags)| Consume the primed token and fetch the new one
|look(checkFlags)| Look at the primed token without consuming it (like peek but without side-effects).
|conumeOnMatch(TokenKind)| Like matchToken but without side-effects.

Changing to the new set of methods lets me make sure I considered everything properly in converting it to the pump priming way of doing things.

Comment 4

[Brendan Eich [:brendan]]

Chris: your call on the inline forwarders -- subtle trade-offs here, not my call.

The new method names start with the pump metaphor but then switch to consumption instead of pumping.

More substantially, why have prime and consume? Typically you just .get() your way along (e.g., in javac, if memory serves), and inspect .token to see what was last got. Does the opportunity for assertions motivates the prime/consume split?

/be

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #4)

Yeah, I'm concerned that the flags that the token was primed with are not the ones expected at the get() site, so this interface helps me check that. |.token| is analogous to |look(assertFlags)| but with flag checking.

There are only a few E4X tests in our test suite where we have more than one token of lookahead. In this case I'm guessing priming twice should be a suitable replacement for multiple |unget()|s.

Comment 6

[Jim Blandy :jimb]

Comment on attachment 433980 [details] [diff] [review]
Switch to TokenStream methods.

Looks good --- just get a full test suite run on it.

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Switch to TokenStream methods.

Slightly stale. Passes full suite, worming its way through the try server now.

Comment 8

[Jim Blandy :jimb]

This has been landed in TM --- isn't that so?
http://hg.mozilla.org/tracemonkey/rev/732dbd2b2d46

If yes, then this can be marked "ASSIGNED" and have "fixed-in-tracemonkey" in the "whiteboard" field now.