Simplify scanner and generate better error message for legacy octals in templates

Simplify scanner and generate better error message for legacy octals in templates

LOG=N
BUG=
R=arv@chromium.org, dslomov@chromium.org

Committed: https://crrev.com/233f2d2bf133fef3d9db5cb8cacc903f1a22b45a
Cr-Commit-Position: refs/heads/master@{#25895}

[caitp (gmail), 2014-12-18 20:29:13]

PTAL

Trying to make octal treatment consistent with behaviour in strict mode,
simplify scanning, and produce a better error message.

https://bugs.ecmascript.org/show_bug.cgi?id=3477 and
https://github.com/rwaldron/tc39-notes/blob/b26f950b438b18ac3497fd717dfb2370a...
discuss this a bit

[arv (Not doing code reviews), 2014-12-18 20:38:51]

https://codereview.chromium.org/812163003/diff/1/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/812163003/diff/1/src/preparser.h#newcode2849
src/preparser.h:2849: CheckOctalLiteral(pos, peek_position(), CHECK_OK);
CheckOctalLiteral needs to be updated to use a different error message.

https://codereview.chromium.org/812163003/diff/1/test/mjsunit/harmony/templat...
File test/mjsunit/harmony/templates.js (right):

https://codereview.chromium.org/812163003/diff/1/test/mjsunit/harmony/templat...
test/mjsunit/harmony/templates.js:482: code = "(function(){})" + code;
Why do we need a tagged version here?

[caitp (gmail), 2014-12-18 20:44:18]

https://codereview.chromium.org/812163003/diff/1/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/812163003/diff/1/src/preparser.h#newcode2849
src/preparser.h:2849: CheckOctalLiteral(pos, peek_position(), CHECK_OK);
On 2014/12/18 20:38:51, arv wrote:
> CheckOctalLiteral needs to be updated to use a different error message.

Yeah --- I was undecided whether it should be something generic like "Octal
escape sequences are deprecated", or let callers pass in their own message ID
and make a template-specific error message.

What do you think?

https://codereview.chromium.org/812163003/diff/1/test/mjsunit/harmony/templat...
File test/mjsunit/harmony/templates.js (right):

https://codereview.chromium.org/812163003/diff/1/test/mjsunit/harmony/templat...
test/mjsunit/harmony/templates.js:482: code = "(function(){})" + code;
On 2014/12/18 20:38:51, arv wrote:
> Why do we need a tagged version here?

When testing, I ran into issues when calling CheckOctalLiteral() because I was
sending the wrong positions, so I thought it would be good to check this case
too just in case something changes there, since it was a bit finicky to make it
work.

[arv (Not doing code reviews), 2014-12-18 20:46:58]

I agree that this is a cleaner approach.

https://codereview.chromium.org/812163003/diff/1/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/812163003/diff/1/src/preparser.h#newcode2849
src/preparser.h:2849: CheckOctalLiteral(pos, peek_position(), CHECK_OK);
On 2014/12/18 20:44:18, caitp wrote:
> On 2014/12/18 20:38:51, arv wrote:
> > CheckOctalLiteral needs to be updated to use a different error message.
> 
> Yeah --- I was undecided whether it should be something generic like "Octal
> escape sequences are deprecated", or let callers pass in their own message ID
> and make a template-specific error message.
> 
> What do you think?

I think a new error message would be best.

https://codereview.chromium.org/812163003/diff/1/test/mjsunit/harmony/templat...
File test/mjsunit/harmony/templates.js (right):

https://codereview.chromium.org/812163003/diff/1/test/mjsunit/harmony/templat...
test/mjsunit/harmony/templates.js:482: code = "(function(){})" + code;
On 2014/12/18 20:44:18, caitp wrote:
> On 2014/12/18 20:38:51, arv wrote:
> > Why do we need a tagged version here?
> 
> When testing, I ran into issues when calling CheckOctalLiteral() because I was
> sending the wrong positions, so I thought it would be good to check this case
> too just in case something changes there, since it was a bit finicky to make
it
> work.

Makes sense

[caitp (gmail), 2014-12-18 21:09:50]

https://codereview.chromium.org/812163003/diff/1/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/812163003/diff/1/src/preparser.h#newcode2849
src/preparser.h:2849: CheckOctalLiteral(pos, peek_position(), CHECK_OK);
On 2014/12/18 20:46:58, arv wrote:
> On 2014/12/18 20:44:18, caitp wrote:
> > On 2014/12/18 20:38:51, arv wrote:
> > > CheckOctalLiteral needs to be updated to use a different error message.
> > 
> > Yeah --- I was undecided whether it should be something generic like "Octal
> > escape sequences are deprecated", or let callers pass in their own message
ID
> > and make a template-specific error message.
> > 
> > What do you think?
> 
> I think a new error message would be best.

Done and done

[arv (Not doing code reviews), 2014-12-18 21:31:53]

LGTM

https://codereview.chromium.org/812163003/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/812163003/diff/20001/src/preparser.h#newcode388
src/preparser.h:388: // If so, reports an error. Only called for strict mode.
update comment too

[caitp (gmail), 2014-12-18 21:34:19]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2014-12-18 21:34:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/812163003/60001

[commit-bot: I haz the power, 2014-12-18 22:01:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-18 22:01:52]

Patchset 4 (id:??) landed as
https://crrev.com/233f2d2bf133fef3d9db5cb8cacc903f1a22b45a
Cr-Commit-Position: refs/heads/master@{#25895}