[Parser] Remove Variable::is_possibly_eval.

[Parser] Remove Variable::is_possibly_eval.

Removes Variable::is_possibly_eval() and instead stores whether
a call is possibly eval in the Call node's bitfield.

Also removes HandleDereferenceMode since it's no longer used.

BUG=v8:5203
Committed: https://crrev.com/160d0a180363b02e5f346db5fd6303d1e9bb9b9a
Cr-Commit-Position: refs/heads/master@{#38633}

[rmcilroy, 2016-08-11 21:27:58]

rmcilroy@chromium.org changed reviewers:
+ adamk@chromium.org, marja@chromium.org, verwaest@chromium.org

[rmcilroy, 2016-08-11 21:27:59]

PTAL, thanks.

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

https://codereview.chromium.org/2242583003/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:5561: Call::PossiblyEval is_possibly_eval =
CheckPossibleEvalCall(tag, scope());
This call updates the scopes to record the eval call - I'm not sure if this is
safe here but doesn't seem to cause any issues on the test. Let me know if we
should do something else.

[adamk, 2016-08-11 21:42:31]

https://codereview.chromium.org/2242583003/diff/1/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2242583003/diff/1/src/ast/ast.h#newcode3247
src/ast/ast.h:3247: int pos, Call::PossiblyEval possibly_eval) {
Can you give this a default of "NOT_EVAL"? That avoids sprinkling it around the
parser.

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

https://codereview.chromium.org/2242583003/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:5561: Call::PossiblyEval is_possibly_eval =
CheckPossibleEvalCall(tag, scope());
On 2016/08/11 21:27:59, rmcilroy wrote:
> This call updates the scopes to record the eval call - I'm not sure if this is
> safe here but doesn't seem to cause any issues on the test. Let me know if we
> should do something else.

There used to be a call to CheckPossibleEvalCall here, but I removed it in
https://codereview.chromium.org/1580463003/. I wish I'd written a better CL
description, though, as I can't remember what caused me to make that change. My
guess is I was worried about the fact that this call only appears in the parser,
not preparser. So a function could get different "uses eval" bits when
preparsing than when doing full parsing. If you're going to add it back in, I
think it ought to also get set when preparsing.

Does anything fail if you leave this out, I'm curious? As noted in my CL, using
eval as a template tag is harmless: it's a call to a runtime function that
returns its argument.

[rmcilroy, 2016-08-12 09:18:19]

rmcilroy@chromium.org changed reviewers:
+ ahaas@chromium.org, epertoso@chromium.org

[rmcilroy, 2016-08-12 09:18:20]

epertoso@chromium.org: Please review changes in compiler/
ahaas@chromium.org: Please review changes in asm/

https://codereview.chromium.org/2242583003/diff/1/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2242583003/diff/1/src/ast/ast.h#newcode3247
src/ast/ast.h:3247: int pos, Call::PossiblyEval possibly_eval) {
On 2016/08/11 21:42:31, adamk wrote:
> Can you give this a default of "NOT_EVAL"? That avoids sprinkling it around
the
> parser.

Done.

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

https://codereview.chromium.org/2242583003/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:5561: Call::PossiblyEval is_possibly_eval =
CheckPossibleEvalCall(tag, scope());
On 2016/08/11 21:42:31, adamk wrote:
> On 2016/08/11 21:27:59, rmcilroy wrote:
> > This call updates the scopes to record the eval call - I'm not sure if this
is
> > safe here but doesn't seem to cause any issues on the test. Let me know if
we
> > should do something else.
> 
> There used to be a call to CheckPossibleEvalCall here, but I removed it in
> https://codereview.chromium.org/1580463003/. I wish I'd written a better CL
> description, though, as I can't remember what caused me to make that change.
My
> guess is I was worried about the fact that this call only appears in the
parser,
> not preparser. So a function could get different "uses eval" bits when
> preparsing than when doing full parsing. If you're going to add it back in, I
> think it ought to also get set when preparsing.
> 
> Does anything fail if you leave this out, I'm curious? As noted in my CL,
using
> eval as a template tag is harmless: it's a call to a runtime function that
> returns its argument.

The reason I added this is that when I took it out the behavior of
Call::is_possibly_eval was different from Variable::is_possibly_eval for the
tests in mjsunit/es6/template.js. Taking it out doesn't seem to have anything to
fail though, so I've taken it out now.

[ahaas, 2016-08-12 09:21:37]

On 2016/08/12 at 09:18:20, rmcilroy wrote:
> epertoso@chromium.org: Please review changes in compiler/
> ahaas@chromium.org: Please review changes in asm/
> 
> https://codereview.chromium.org/2242583003/diff/1/src/ast/ast.h
> File src/ast/ast.h (right):
> 
> https://codereview.chromium.org/2242583003/diff/1/src/ast/ast.h#newcode3247
> src/ast/ast.h:3247: int pos, Call::PossiblyEval possibly_eval) {
> On 2016/08/11 21:42:31, adamk wrote:
> > Can you give this a default of "NOT_EVAL"? That avoids sprinkling it around
the
> > parser.
> 
> Done.
> 
> https://codereview.chromium.org/2242583003/diff/1/src/parsing/parser.cc
> File src/parsing/parser.cc (right):
> 
>
https://codereview.chromium.org/2242583003/diff/1/src/parsing/parser.cc#newco...
> src/parsing/parser.cc:5561: Call::PossiblyEval is_possibly_eval =
CheckPossibleEvalCall(tag, scope());
> On 2016/08/11 21:42:31, adamk wrote:
> > On 2016/08/11 21:27:59, rmcilroy wrote:
> > > This call updates the scopes to record the eval call - I'm not sure if
this is
> > > safe here but doesn't seem to cause any issues on the test. Let me know if
we
> > > should do something else.
> > 
> > There used to be a call to CheckPossibleEvalCall here, but I removed it in
> > https://codereview.chromium.org/1580463003/. I wish I'd written a better CL
> > description, though, as I can't remember what caused me to make that change.
My
> > guess is I was worried about the fact that this call only appears in the
parser,
> > not preparser. So a function could get different "uses eval" bits when
> > preparsing than when doing full parsing. If you're going to add it back in,
I
> > think it ought to also get set when preparsing.
> > 
> > Does anything fail if you leave this out, I'm curious? As noted in my CL,
using
> > eval as a template tag is harmless: it's a call to a runtime function that
> > returns its argument.
> 
> The reason I added this is that when I took it out the behavior of
Call::is_possibly_eval was different from Variable::is_possibly_eval for the
tests in mjsunit/es6/template.js. Taking it out doesn't seem to have anything to
fail though, so I've taken it out now.

wasm lgtm

[marja, 2016-08-12 09:27:54]

lgtm; great to get rid of the handledereferencemode!

[epertoso, 2016-08-12 09:40:22]

lgtm

 for compiler/

[Toon Verwaest, 2016-08-12 09:47:14]

nice! lgtm!

[rmcilroy, 2016-08-15 10:32:29]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2016-08-15 10:32:31]

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, 2016-08-15 10:58:58]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-15 10:59:25]

Description was changed from

==========
[Parser] Remove Variable::is_possibly_eval.

Removes Variable::is_possibly_eval() and instead stores whether
a call is possibly eval in the Call node's bitfield.

Also removes HandleDereferenceMode since it's no longer used.

BUG=v8:5203
==========

to

==========
[Parser] Remove Variable::is_possibly_eval.

Removes Variable::is_possibly_eval() and instead stores whether
a call is possibly eval in the Call node's bitfield.

Also removes HandleDereferenceMode since it's no longer used.

BUG=v8:5203

Committed: https://crrev.com/160d0a180363b02e5f346db5fd6303d1e9bb9b9a
Cr-Commit-Position: refs/heads/master@{#38633}
==========

[commit-bot: I haz the power, 2016-08-15 10:59:26]

Patchset 2 (id:??) landed as
https://crrev.com/160d0a180363b02e5f346db5fd6303d1e9bb9b9a
Cr-Commit-Position: refs/heads/master@{#38633}