[parser] Correctly handle invalid escapes in adjacent template tokens.

[parser] Correctly handle invalid escapes in adjacent template tokens.

A previous patch lifting the restriction on invalid escape sequences in
tagged templates had a bug when two template tokens appeared immediately
adject to each other. This moves invalid escape information from the
tokenizer state proper into the TokenDesc, preventing the overwriting
which caused this issue.

Previous CL is at
https://codereview.chromium.org/2665513002

BUG=v8:6029, v8:5546
Review-Url: https://codereview.chromium.org/2724003006
Cr-Commit-Position: refs/heads/master@{#43596}
Committed: https://chromium.googlesource.com/v8/v8/+/baa74e89b6927917f911aab0e0823c8e0234aaa2

[bakkot1, 2017-03-02 22:08:25]

Description was changed from

==========
[parser] Correctly handle invalid escapes in adjacent template tokens.

A previous patch lifting the restriction on invalid escape sequences in
tagged templates had a bug when two template tokens appeared immediately
adject to each other. This moves invalid escape information from the
tokenizer state proper into the TokenDesc, preventing the overwriting
which caused this issue.

BUG=v8:6029
==========

to

==========
[parser] Correctly handle invalid escapes in adjacent template tokens.

A previous patch lifting the restriction on invalid escape sequences in
tagged templates had a bug when two template tokens appeared immediately
adject to each other. This moves invalid escape information from the
tokenizer state proper into the TokenDesc, preventing the overwriting
which caused this issue.

Previous CL is at
https://codereview.chromium.org/2665513002

BUG=v8:6029,v8:5546
==========

[bakkot1, 2017-03-02 22:08:25]

bakkot@gmail.com changed reviewers:
+ vogelheim@chromium.org

[bakkot1, 2017-03-02 22:13:04]

PTAL. (Fixes a bug introduced by previous patch.)

I forgot that the lookahead token could be scanned before
the parser had a chance to check for errors on the current token,
so it could get overwritten. Keeping information in the TokenDesc
is probably a better better anyway.

[vogelheim, 2017-03-03 10:40:08]

lgtm

Thanks for DCHECKs + test cases!

https://codereview.chromium.org/2724003006/diff/1/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/2724003006/diff/1/src/parsing/scanner.cc#newc...
src/parsing/scanner.cc:1126: scanner_error_state.MoveErrorTo(
nitpick: With this change, it might be slightly nicer to change the signature of
MoveErrorTo to MoveErrorTo(TokenDesc&), since now there's a 'thing' that
contains both of the variables we're trying to modify here.

https://codereview.chromium.org/2724003006/diff/1/src/parsing/scanner.h
File src/parsing/scanner.h (right):

https://codereview.chromium.org/2724003006/diff/1/src/parsing/scanner.h#newco...
src/parsing/scanner.h:457: Location invalid_template_escape_location_;
style nitpick: The style guide wants struct members to be named without an
underscore.


Ref: https://google.github.io/styleguide/cppguide.html#Variable_Names
  - section "Struct Data Members"
  - referenced from V8 C++ style guide 
    (https://github.com/v8/v8/wiki/Cpp-Style-Guide)
    via Chromium C++ style guide
   
(https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md)


(Yeah, "smi_value_" should be "smi_value". Whatever.)

[vogelheim, 2017-03-03 10:40:23]

The CQ bit was checked by vogelheim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 10:40:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 11:05:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-03 11:05:48]

Dry run: This issue passed the CQ dry run.

[bakkot1, 2017-03-03 20:50:17]

https://codereview.chromium.org/2724003006/diff/1/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/2724003006/diff/1/src/parsing/scanner.cc#newc...
src/parsing/scanner.cc:1126: scanner_error_state.MoveErrorTo(
On 2017/03/03 10:40:08, vogelheim wrote:
> nitpick: With this change, it might be slightly nicer to change the signature
of
> MoveErrorTo to MoveErrorTo(TokenDesc&), since now there's a 'thing' that
> contains both of the variables we're trying to modify here. 

TokenDesc is private in Scanner. I could make it public, I guess.

On the other hand, at that point it could just be "MoveErrorToNextTokenDesc"
and take no parameters at all. Thoughts?

https://codereview.chromium.org/2724003006/diff/1/src/parsing/scanner.h
File src/parsing/scanner.h (right):

https://codereview.chromium.org/2724003006/diff/1/src/parsing/scanner.h#newco...
src/parsing/scanner.h:457: Location invalid_template_escape_location_;
On 2017/03/03 10:40:08, vogelheim wrote:
> style nitpick: The style guide wants struct members to be named without an
> underscore.
> 
> 
> Ref: https://google.github.io/styleguide/cppguide.html#Variable_Names
>   - section "Struct Data Members"
>   - referenced from V8 C++ style guide 
>     (https://github.com/v8/v8/wiki/Cpp-Style-Guide)
>     via Chromium C++ style guide
>    
>
(https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md)
> 
> 
> (Yeah, "smi_value_" should be "smi_value". Whatever.)

Done.

[vogelheim, 2017-03-03 21:26:19]

lgtm

https://codereview.chromium.org/2724003006/diff/1/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/2724003006/diff/1/src/parsing/scanner.cc#newc...
src/parsing/scanner.cc:1126: scanner_error_state.MoveErrorTo(
On 2017/03/03 20:50:17, bakkot1 wrote:
> On 2017/03/03 10:40:08, vogelheim wrote:
> > nitpick: With this change, it might be slightly nicer to change the
signature
> of
> > MoveErrorTo to MoveErrorTo(TokenDesc&), since now there's a 'thing' that
> > contains both of the variables we're trying to modify here. 
> 
> TokenDesc is private in Scanner. I could make it public, I guess.
> 
> On the other hand, at that point it could just be "MoveErrorToNextTokenDesc"
> and take no parameters at all. Thoughts?

Oops. Hrmm, yeah, I think I asked you to take ErrorState out of the header...
:-/


Alternatives:
- Leave things as they are.
- Make TokenDesc public. I'm not fond of it.
- Make ErrorState a friend of Scanner.
- Forward declare Scanner::ErrorState.

I think I'd prefer last or second-to-last, but am happy to let you have the last
word.

(What I mean with the last one is:
Declare: class ErrorState; in the private:-section of Scanner, and in Scanner.cc
implement it as Scanner::ErrorState. When I try it locally (without the rest of
your patch), this seems to work and gives Scanner::ErrorState full access to
Scanner, without polluting the header.)

[bakkot1, 2017-03-03 21:43:56]

On 2017/03/03 21:26:19, vogelheim wrote:
> lgtm
> 
> https://codereview.chromium.org/2724003006/diff/1/src/parsing/scanner.cc
> File src/parsing/scanner.cc (right):
> 
>
https://codereview.chromium.org/2724003006/diff/1/src/parsing/scanner.cc#newc...
> src/parsing/scanner.cc:1126: scanner_error_state.MoveErrorTo(
> On 2017/03/03 20:50:17, bakkot1 wrote:
> > On 2017/03/03 10:40:08, vogelheim wrote:
> > > nitpick: With this change, it might be slightly nicer to change the
> signature
> > of
> > > MoveErrorTo to MoveErrorTo(TokenDesc&), since now there's a 'thing' that
> > > contains both of the variables we're trying to modify here. 
> > 
> > TokenDesc is private in Scanner. I could make it public, I guess.
> > 
> > On the other hand, at that point it could just be "MoveErrorToNextTokenDesc"
> > and take no parameters at all. Thoughts?
> 
> Oops. Hrmm, yeah, I think I asked you to take ErrorState out of the header...
> :-/
> 
> 
> Alternatives:
> - Leave things as they are.
> - Make TokenDesc public. I'm not fond of it.
> - Make ErrorState a friend of Scanner.
> - Forward declare Scanner::ErrorState.
> 
> I think I'd prefer last or second-to-last, but am happy to let you have the
last
> word.
> 
> (What I mean with the last one is:
> Declare: class ErrorState; in the private:-section of Scanner, and in
Scanner.cc
> implement it as Scanner::ErrorState. When I try it locally (without the rest
of
> your patch), this seems to work and gives Scanner::ErrorState full access to
> Scanner, without polluting the header.)

Went with forward declaration. It seemed cleanest.

[bakkot1, 2017-03-03 21:44:05]

The CQ bit was checked by bakkot@gmail.com

[bakkot1, 2017-03-03 21:44:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2724003006/#ps40001
(title: "MoveErrorTo takes a TokenDesc*")

[commit-bot: I haz the power, 2017-03-03 21:44:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 22:08:57]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488577445932860,
"parent_rev": "3a20c322bbc26b453ce9c3c3b83094af2fb722f2", "commit_rev":
"baa74e89b6927917f911aab0e0823c8e0234aaa2"}

[commit-bot: I haz the power, 2017-03-03 22:09:03]

Description was changed from

==========
[parser] Correctly handle invalid escapes in adjacent template tokens.

A previous patch lifting the restriction on invalid escape sequences in
tagged templates had a bug when two template tokens appeared immediately
adject to each other. This moves invalid escape information from the
tokenizer state proper into the TokenDesc, preventing the overwriting
which caused this issue.

Previous CL is at
https://codereview.chromium.org/2665513002

BUG=v8:6029,v8:5546
==========

to

==========
[parser] Correctly handle invalid escapes in adjacent template tokens.

A previous patch lifting the restriction on invalid escape sequences in
tagged templates had a bug when two template tokens appeared immediately
adject to each other. This moves invalid escape information from the
tokenizer state proper into the TokenDesc, preventing the overwriting
which caused this issue.

Previous CL is at
https://codereview.chromium.org/2665513002

BUG=v8:6029,v8:5546

Review-Url: https://codereview.chromium.org/2724003006
Cr-Commit-Position: refs/heads/master@{#43596}
Committed:
https://chromium.googlesource.com/v8/v8/+/baa74e89b6927917f911aab0e0823c8e023...
==========

[commit-bot: I haz the power, 2017-03-03 22:09:04]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/baa74e89b6927917f911aab0e0823c8e023...