templates: Don't check IsLineTerminator() if character is negative

templates: Don't check IsLineTerminator() if character is negative

BUG=455212
LOG=N
R=arv@chromium.org

Committed: https://crrev.com/49ef5498075fbc2ce09dbfa76ecb51b39464df10
Cr-Commit-Position: refs/heads/master@{#26442}

[caitp (gmail), 2015-02-04 20:07:56]

PTAL

I'm not sure this is the best way to fix this

[arv (Not doing code reviews), 2015-02-04 20:10:49]

LGTM

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

https://codereview.chromium.org/902703002/diff/1/src/scanner.cc#newcode846
src/scanner.cc:846: if (c0_ > 0 && unicode_cache_->IsLineTerminator(c0_)) {
This seems fine.

One thing we can do is to add a DCHECK to IsLineTerminator.

[caitp (gmail), 2015-02-04 20:19:36]

On 2015/02/04 20:10:49, arv wrote:
> LGTM
> 
> https://codereview.chromium.org/902703002/diff/1/src/scanner.cc
> File src/scanner.cc (right):
> 
> https://codereview.chromium.org/902703002/diff/1/src/scanner.cc#newcode846
> src/scanner.cc:846: if (c0_ > 0 && unicode_cache_->IsLineTerminator(c0_)) {
> This seems fine.
> 
> One thing we can do is to add a DCHECK to IsLineTerminator.

Should I do that here?

Question: does the fuzztester care if a test fails? because, it looks to me like
even before, it should never have gotten past that one line, due to the syntax
error and exception. (that's why I didn't just copy in the regression test,
since it would have failed if the test-runner ran it)

[arv (Not doing code reviews), 2015-02-04 20:26:37]

On 2015/02/04 20:19:36, caitp wrote:
> On 2015/02/04 20:10:49, arv wrote:
> > LGTM
> > 
> > https://codereview.chromium.org/902703002/diff/1/src/scanner.cc
> > File src/scanner.cc (right):
> > 
> > https://codereview.chromium.org/902703002/diff/1/src/scanner.cc#newcode846
> > src/scanner.cc:846: if (c0_ > 0 && unicode_cache_->IsLineTerminator(c0_)) {
> > This seems fine.
> > 
> > One thing we can do is to add a DCHECK to IsLineTerminator.
> 
> Should I do that here?

At least that would have caught this issue on the debug bots. Maybe it would
have slowed them down too much though?

Lets submit it without the DCHECK so that we can get this into the release
branch.

> Question: does the fuzztester care if a test fails? because, it looks to me
like
> even before, it should never have gotten past that one line, due to the syntax
> error and exception. (that's why I didn't just copy in the regression test,
> since it would have failed if the test-runner ran it)

The fuzztest looks OK to me. The first assertEquals passes.

[caitp (gmail), 2015-02-04 20:41:48]

caitpotter88@gmail.com changed reviewers:
- marja@chromium.org

[caitp (gmail), 2015-02-04 20:42:13]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-02-04 20:42:20]

The CQ bit was unchecked by caitpotter88@gmail.com

[caitp (gmail), 2015-02-04 20:42:29]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2015-02-04 20:43:07]

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

[commit-bot: I haz the power, 2015-02-04 21:05:41]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-02-04 21:05:59]

Patchset 1 (id:??) landed as
https://crrev.com/49ef5498075fbc2ce09dbfa76ecb51b39464df10
Cr-Commit-Position: refs/heads/master@{#26442}