Use a single Token::INIT for all variable initialization

Use a single Token::INIT for all variable initialization

All uses of Token::INIT also have access to the relevant VariableMode,
so there's no reason to have more than one token representing an
initializing assignment.

Committed: https://crrev.com/462cc3c6f05d1594df89ff62e0d5bc472888f9ff
Cr-Commit-Position: refs/heads/master@{#32016}

[adamk, 2015-11-10 19:51:44]

adamk@chromium.org changed reviewers:
+ rossberg@chromium.org

[rossberg, 2015-11-12 16:37:00]

I like this change, but I agree that Token is a bit of an odd place to put the
function. How about ast.h?

Actually, this makes me wonder why we need the different INIT tokens at all.
Shouldn't it be possible to have just one, and distinguish by VariableMode?

[Michael Starzinger, 2015-11-12 16:41:06]

On 2015/11/12 16:37:00, rossberg wrote:
> I like this change, but I agree that Token is a bit of an odd place to put the
> function. How about ast.h?
> 
> Actually, this makes me wonder why we need the different INIT tokens at all.
> Shouldn't it be possible to have just one, and distinguish by VariableMode?

Yeah, I was asking myself the same thing several times. Looking at
AstGraphBuilder::BuildVariableAssignment for example, the variable mode and the
token always come on pairs. AFAICT one "init" token oughta be enough for the
compilers, not sure about phases that run before the compilers though.

[adamk, 2015-11-13 06:48:52]

On 2015/11/12 16:41:06, Michael Starzinger wrote:
> On 2015/11/12 16:37:00, rossberg wrote:
> > I like this change, but I agree that Token is a bit of an odd place to put
the
> > function. How about ast.h?
> > 
> > Actually, this makes me wonder why we need the different INIT tokens at all.
> > Shouldn't it be possible to have just one, and distinguish by VariableMode?
> 
> Yeah, I was asking myself the same thing several times. Looking at
> AstGraphBuilder::BuildVariableAssignment for example, the variable mode and
the
> token always come on pairs. AFAICT one "init" token oughta be enough for the
> compilers, not sure about phases that run before the compilers though.

I liked this idea, so I gave it a shot. See patch set 2. I already ran into one
pre-existing bug (fixed in https://codereview.chromium.org/1437003006/, which
needs to land before this one). The form of that bug, which this patch revealed,
was that we were trying to use INIT_VAR to initialize a CONST-mode variable. I
don't think such a thing should ever happen  (and the patch I linked to fixes
this case), but it does point to fact that these differing INIT tokens might
provide something of a safety valve. We'll see what the full suite of trybots
think of this patch.

[rossberg, 2015-11-13 07:55:23]

LGTM. You might want to update the CL description, though.

https://codereview.chromium.org/1431873006/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1431873006/diff/20001/src/parser.cc#newcode4810
src/parser.cc:4810: const bool use_strict_const =
is_strict(scope_->language_mode());
Nit: could now merge this into the next declaration.

[Michael Starzinger, 2015-11-13 08:50:37]

mstarzinger@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Michael Starzinger, 2015-11-13 08:50:38]

LGTM on "compiler" and "full-codegen". I like it.

https://codereview.chromium.org/1431873006/diff/20001/src/full-codegen/arm/fu...
File src/full-codegen/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1431873006/diff/20001/src/full-codegen/arm/fu...
src/full-codegen/arm/full-codegen-arm.cc:2625: } else if (var->is_this() &&
var->mode() == CONST) {
nit: It might make sense to also check {op == Token::INIT} here, for safety,
even though we should never get here for non-INIT const assignment. On the other
architectures you seem to already have done so.

[adamk, 2015-11-13 19:04:32]

Description was changed from

==========
Add Token::InitOp to translate VariableMode to Token::Value

This keeps each bit of code that wants to declare and initialize
a variable from having to figure out which INIT_* token to use.

The layering is a little funny, since Token shouldn't know
much about VariableMode, but it seemed like the obvious
place to put the helper method.
==========

to

==========
Use a single Token::INIT for all variable initialization

All uses of Token::INIT also have access to the relevant VariableMode,
so there's no reason to have more than one token representing an
initializing assignment.
==========

[adamk, 2015-11-13 19:05:50]

Updated CL description, handled comments. Will land once the eval test is fixed
(by my other change).

https://codereview.chromium.org/1431873006/diff/20001/src/full-codegen/arm/fu...
File src/full-codegen/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1431873006/diff/20001/src/full-codegen/arm/fu...
src/full-codegen/arm/full-codegen-arm.cc:2625: } else if (var->is_this() &&
var->mode() == CONST) {
On 2015/11/13 08:50:38, Michael Starzinger wrote:
> nit: It might make sense to also check {op == Token::INIT} here, for safety,
> even though we should never get here for non-INIT const assignment. On the
other
> architectures you seem to already have done so.

Fixed, this was just a copy/paste fail.

https://codereview.chromium.org/1431873006/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1431873006/diff/20001/src/parser.cc#newcode4810
src/parser.cc:4810: const bool use_strict_const =
is_strict(scope_->language_mode());
On 2015/11/13 07:55:23, rossberg wrote:
> Nit: could now merge this into the next declaration.

Done.

[adamk, 2015-11-13 21:26:44]

The CQ bit was checked by adamk@chromium.org

[adamk, 2015-11-13 21:26:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1431873006/#ps40001
(title: "Fix arm, simplify fvar code")

[commit-bot: I haz the power, 2015-11-13 21:26:54]

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

[commit-bot: I haz the power, 2015-11-13 21:29:25]

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

[commit-bot: I haz the power, 2015-11-13 21:29:25]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/7732)

[adamk, 2015-11-13 21:30:47]

adamk@chromium.org changed reviewers:
+ bmeurer@chromium.org

[adamk, 2015-11-13 21:30:47]

+bmeurer for crankshaft/OWNERS

[Benedikt Meurer, 2015-11-15 18:54:35]

Nice cleanup. LGTM.

[adamk, 2015-11-16 16:44:44]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2015-11-16 16:45:03]

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

[commit-bot: I haz the power, 2015-11-16 17:07:51]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-16 17:08:16]

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