I've never quite understood peekTokenSameLine(), and I now know why -- it's bogus. It's supposed to return TOK_EOL if there's a newline between the end of the current token and the start of the next token. But consider the very start of it: TokenKind peekTokenSameLine(unsigned withFlags = 0) { if (!onCurrentLine(currentToken().pos)) return TOK_EOL; This tests that the *end* of the *furthest-scanned* token is on the same line as the end of the current token, instead of testing that the *start* of the *next* token is on the same line. I.e. it's wrong in two cases. - If the next token is a multi-line token. - If we've scanned ahead *two* tokens and there's a newline between the next token and the one after that. These cases are pretty obscure and I've only managed to find one instance where the first wrong case occurs in practice (which I'll describe shortly), and no instances of the second.

This patch fixes the problem, and adds copious comments. The fix allows the TSF_EOL flag to be removed, which is nice.

Here's the test case where this produces different behaviour: module 'ma\ th' { export function sum(x, y) { return x + y; } } The issue is that the module name needs to be on the same line as the |module| keyword, and it is, but because the name is a multi-line string the old code thinks it isn't. Output with the old code: m.js:2:4 SyntaxError: missing ; before statement: m.js:2:4 th' { m.js:2:4 ....^ Output with my patch applied: m.js:3:4 SyntaxError: export is a reserved identifier: m.js:3:4 export function sum(x, y) { return x + y; } m.js:3:4 ....^ Now, something's a bit screwy with modules -- |module| is accepted as a keyword but |export| isn't. I'm not sure what's going on there, which is why I haven't actually written a test as part of this patch, but I'm pretty sure the new behaviour is more correct. Eddy, can you explain this? Are modules only partly implemented?

I noticed these comment errors while I was in here.

Comment on attachment 784223 [details] [diff] [review] (part 1) - Fix peekTokenSameLine(). Review of attachment 784223 [details] [diff] [review]: ----------------------------------------------------------------- Modules are only partially implemented, yes, so you might not be able to write an actual testcase for this. ::: js/src/frontend/TokenStream.h @@ +321,5 @@ > + TSF_OPERAND = 0x02, /* looking for operand, not operator */ > + TSF_UNEXPECTED_EOF = 0x04, /* unexpected end of input, i.e. TOK_EOF not at top-level. */ > + TSF_KEYWORD_IS_NAME = 0x08, /* Ignore keywords and return TOK_NAME instead to the parser. */ > + TSF_DIRTYLINE = 0x10, /* non-whitespace since start of line */ > + TSF_OCTAL_CHAR = 0x20, /* observed a octal character escape */ If you're touching all these, make this "an". Although, I'd have just moved the trailing initializers up to the empty/removed spaces, myself...

> Modules are only partially implemented, yes, so you might not be able to > write an actual testcase for this. Ok, I'll clear the needinfo. Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/31796ab06f39 https://hg.mozilla.org/integration/mozilla-inbound/rev/8dcd80a370e0

https://hg.mozilla.org/mozilla-central/rev/31796ab06f39 https://hg.mozilla.org/mozilla-central/rev/8dcd80a370e0