[parser] fix token end position for regexp literals

[parser] fix token end position for regexp literals

Make the end position of a regexp literal the first character following the regexp. This matches the behaviour of number literals and string literals, as well as single-character tokens.

This change corrects the lazy-parsing of arrow functions with concise bodies, whose last token is a regular expression literal.

BUG=v8:4474
LOG=N
R=wingo@igalia.com, adamk@chromium.org, rossberg@chromium.org

Committed: https://crrev.com/e2be5ce542b76637efa5f984f2413d85951d5077
Cr-Commit-Position: refs/heads/master@{#31189}

[caitp (gmail), 2015-10-07 15:47:04]

PTAL

Just going through the available bugs, and this one looked pretty simple to
solve.

As far as I can tell from testing, this should never cause any problems.

[caitp (gmail), 2015-10-07 15:47:58]

On 2015/10/07 15:47:04, caitp wrote:
> PTAL
> 
> Just going through the available bugs, and this one looked pretty simple to
> solve.
> 
> As far as I can tell from testing, this should never cause any problems.

However, "make check" does report some problems with this, I'll take another
look after the meeting.

[caitp (gmail), 2015-10-07 17:45:15]

On 2015/10/07 15:47:58, caitp wrote:
> On 2015/10/07 15:47:04, caitp wrote:
> > PTAL
> > 
> > Just going through the available bugs, and this one looked pretty simple to
> > solve.
> > 
> > As far as I can tell from testing, this should never cause any problems.
> 
> However, "make check" does report some problems with this, I'll take another
> look after the meeting.

Okay, this version of the fix looks a tiny bit suspicious, but as far as I can
tell it does the right thing, and seems to pass all tests? If it's good for
test262 and everything, this is probably the one.

[adamk, 2015-10-07 18:12:16]

Did you do any git blame archaeology to see where that "- 1" came from? This
definitely looks like a shockingly general change (and I'm surprised it doesn't
break something else).

[caitp (gmail), 2015-10-07 18:14:19]

On 2015/10/07 18:12:16, adamk wrote:
> Did you do any git blame archaeology to see where that "- 1" came from? This
> definitely looks like a shockingly general change (and I'm surprised it
doesn't
> break something else).

I was taking a look before the meeting. I don't think it breaks anything else
because CurrentSymbol() still has the right source and right flags, and the end
position of the token wasn't used as the end position of a scope anywhere else
before.

The fix still seems kind of suspicious to me, but I haven't been able to fix it
correctly any other way yet

[adamk, 2015-10-07 22:34:38]

On 2015/10/07 18:14:19, caitp wrote:
> On 2015/10/07 18:12:16, adamk wrote:
> > Did you do any git blame archaeology to see where that "- 1" came from? This
> > definitely looks like a shockingly general change (and I'm surprised it
> doesn't
> > break something else).
> 
> I was taking a look before the meeting. I don't think it breaks anything else
> because CurrentSymbol() still has the right source and right flags, and the
end
> position of the token wasn't used as the end position of a scope anywhere else
> before.
> 
> The fix still seems kind of suspicious to me, but I haven't been able to fix
it
> correctly any other way yet

I see why this fix fixes the bug, but what I don't understand is why were were
off-by-one to begin with. It seems like the right fix for this would target that
off-by-one error, rather than patching it elsewhere.

[caitp (gmail), 2015-10-07 23:00:48]

On 2015/10/07 22:34:38, adamk wrote:
> On 2015/10/07 18:14:19, caitp wrote:
> > On 2015/10/07 18:12:16, adamk wrote:
> > > Did you do any git blame archaeology to see where that "- 1" came from?
This
> > > definitely looks like a shockingly general change (and I'm surprised it
> > doesn't
> > > break something else).
> > 
> > I was taking a look before the meeting. I don't think it breaks anything
else
> > because CurrentSymbol() still has the right source and right flags, and the
> end
> > position of the token wasn't used as the end position of a scope anywhere
else
> > before.
> > 
> > The fix still seems kind of suspicious to me, but I haven't been able to fix
> it
> > correctly any other way yet
> 
> I see why this fix fixes the bug, but what I don't understand is why were were
> off-by-one to begin with. It seems like the right fix for this would target
that
> off-by-one error, rather than patching it elsewhere.

Well, that scanner fixes an actual bug in the scanner that just wasn't
noticeable before, when no flags are used. But, since it's broken when flags are
used too, special casing it to only fix the no-flag version doesn't make sense.
It's possible to solve the problem in the parser, but much hackier and more
complicated

[adamk, 2015-10-07 23:37:39]

Ah, I misunderstood (hadn't gone back to re-read what the parser does for
regexps).

This lgtm

[adamk, 2015-10-07 23:45:24]

Oh, but please update the CL description (including the first line) to make it
clear this is narrowly-targeted at RegExp parsing.

[caitp (gmail), 2015-10-08 04:19:00]

On 2015/10/07 23:45:24, adamk wrote:
> Oh, but please update the CL description (including the first line) to make it
> clear this is narrowly-targeted at RegExp parsing.

I've clarified it --- I guess it's not really fixing a regexp parsing bug, but
it is the simplest fix for the arrow scope bug

[caitp (gmail), 2015-10-09 00:38:19]

On 2015/10/08 04:19:00, caitp wrote:
> On 2015/10/07 23:45:24, adamk wrote:
> > Oh, but please update the CL description (including the first line) to make
it
> > clear this is narrowly-targeted at RegExp parsing.
> 
> I've clarified it --- I guess it's not really fixing a regexp parsing bug, but
> it is the simplest fix for the arrow scope bug

I thought it might be the bad fix for this, but this actually makes the
behaviour more consistent with other literal tokens.

"foo" -> { beg_pos = 0, end_pos = 5 }
/foo/ -> { beg_pos = 0, end_pos = 5 } (with patch), { beg_pos = 0, end_pos = 4 }
(without patch)
12345 -> { beg_pos = 0, end_pos = 5 }

So the convention seems to be, end_pos === the index of the first character
after the token. I feel better about landing this now.

[caitp (gmail), 2015-10-09 00:41:27]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2015-10-09 00:41:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1389313003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1389313003/40001

[commit-bot: I haz the power, 2015-10-09 01:02:02]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-09 01:02:13]

Patchset 3 (id:??) landed as
https://crrev.com/e2be5ce542b76637efa5f984f2413d85951d5077
Cr-Commit-Position: refs/heads/master@{#31189}