[strong] Disallow implicit conversions for binary arithmetic operations

[strong] Disallow implicit conversions for binary arithmetic operations

Implements the strong mode proposal's restrictions on
implicit conversions for binary arithmetic operations, not
including the + special case. Adds some infrastructure
for future implementation of the restrictions for other
operators.

BUG=v8:3956
LOG=N

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

[conradw, 2015-04-20 15:29:38]

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

[conradw, 2015-04-20 16:36:17]

On 2015/04/20 15:29:38, conradw wrote:
> mailto:conradw@chromium.org changed reviewers:
> + mailto:rossberg@chromium.org

PTAL

[Sven Panne, 2015-04-21 07:00:36]

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

[Sven Panne, 2015-04-21 07:00:37]

Tiny DBC...

https://codereview.chromium.org/1092353002/diff/40001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/1092353002/diff/40001/src/code-stubs.h#newcod...
src/code-stubs.h:1251: BinaryOpICStub(Isolate* isolate, Token::Value op, bool
strong)
Boolean parameters are discouraged for various reasons (readability of the call
sites + being able to pass almost everything without noticing), so what about
using e.g. LanguageMode? Or are we already running out of bits in the state?

This holds for other places of this CL, too.

[conradw, 2015-04-22 15:48:54]

Patchset #1 (id:1) has been deleted

[conradw, 2015-04-22 15:49:03]

Patchset #1 (id:20001) has been deleted

[conradw, 2015-04-22 15:49:12]

Patchset #1 (id:40001) has been deleted

[conradw, 2015-04-22 15:53:58]

Patchset #1 (id:60001) has been deleted

[conradw, 2015-04-22 16:00:41]

Patchset #1 (id:80001) has been deleted

[conradw, 2015-04-22 16:04:34]

Patchset #1 (id:100001) has been deleted

[conradw, 2015-04-22 17:51:59]

Patchset #2 (id:140001) has been deleted

[conradw, 2015-04-22 17:53:09]

Patchset #1 (id:120001) has been deleted

[conradw, 2015-04-22 19:14:50]

PTAL

I've got rid of the previous patchsets since this attempt is mostly a new
approach, the diffs just looked a bit silly.

I implemented the bool strong -> LanguageMode change as suggested.

[conradw, 2015-04-22 19:40:25]

Patchset #2 (id:180001) has been deleted

[rossberg, 2015-04-23 13:18:09]

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

[rossberg, 2015-04-23 13:29:37]

Looks pretty good!

Adding mstarzinger to sign off the TF changes.

Sven, see my first comment regarding the language mode as a key to the
BinaryICOpState -- I think that would likely cause unnecessary regressions. The
question is at which level to do the mode->bool projection; would you prefer
something else than what I suggested?

https://codereview.chromium.org/1092353002/diff/240001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/1092353002/diff/240001/src/code-stubs.h#newco...
src/code-stubs.h:1253: BinaryOpICState state(isolate, op, language_mode);
I think it's right for the BinoryOpICStub constructor to take the language mode.
However, BinaryOpICState should only take a bool, since otherwise we'd start
distinguishing sloppy vs strict in the state and hence stub key, which implies
that we'll cache them separately.

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/js-typed-...
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/js-typed-...
src/compiler/js-typed-lowering.cc:347: return ReduceStrongNumberBinop(node,
numberOp);
I'd just inline the function.

https://codereview.chromium.org/1092353002/diff/240001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/1092353002/diff/240001/src/ic/ic.cc#newcode2817
src/ic/ic.cc:2817: return is_strong(language_mode) ? Builtins::SUB_STRONG :
Builtins::SUB;
Nit: considering that you'll have to add the same to every case eventually, it
seems better to transpose the switch and conditional.

Btw, all the breaks in this switch can be removed.

https://codereview.chromium.org/1092353002/diff/240001/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/1092353002/diff/240001/src/messages.js#newcod...
src/messages.js:135: strong_implicit_cast:          ["In strong mode, implicit
casts are deprecated"],
Nit: "casts" -> "conversions"

https://codereview.chromium.org/1092353002/diff/240001/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1092353002/diff/240001/src/runtime.js#newcode202
src/runtime.js:202: //ECMA-262, section 11.6.2, page 50.
Nit: spacing (here and below)

https://codereview.chromium.org/1092353002/diff/240001/test/cctest/cctest.h
File test/cctest/cctest.h (right):

https://codereview.chromium.org/1092353002/diff/240001/test/cctest/cctest.h#n...
test/cctest/cctest.h:40: #ifndef TEST_WITH_STRONG
I'd prefer to move this elsewhere, it is a bit too specialised for this header.

https://codereview.chromium.org/1092353002/diff/240001/test/cctest/compiler/t...
File test/cctest/compiler/test-js-typed-lowering.cc (right):

https://codereview.chromium.org/1092353002/diff/240001/test/cctest/compiler/t...
test/cctest/compiler/test-js-typed-lowering.cc:1062: printf("%s\n%s\n", R.ops[j
+ 1]->mnemonic(), B.result->op()->mnemonic());
Stray debug output?

https://codereview.chromium.org/1092353002/diff/240001/test/mjsunit/strong/im...
File test/mjsunit/strong/implicit-casts.js (right):

https://codereview.chromium.org/1092353002/diff/240001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-casts.js:1: // Copyright 2015 the V8 project
authors. All rights reserved.
Nit: rename file to implicit-conversions.js

https://codereview.chromium.org/1092353002/diff/240001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-casts.js:9: //TODO(conradw): Implement other strong
operators
Nit: spacing

https://codereview.chromium.org/1092353002/diff/240001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-casts.js:17: let cast_values = [
Nit: nonnumber_values

https://codereview.chromium.org/1092353002/diff/240001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-casts.js:27: let uncast_values = [
Nit: number_values

https://codereview.chromium.org/1092353002/diff/240001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-casts.js:95: for (let v_left of uncast_values) {
Nit: drop the v_

https://codereview.chromium.org/1092353002/diff/240001/test/unittests/compile...
File test/unittests/compiler/js-operator-unittest.cc (right):

https://codereview.chromium.org/1092353002/diff/240001/test/unittests/compile...
test/unittests/compiler/js-operator-unittest.cc:33: SharedOperator(const
Operator* (JSOperatorBuilder::*cached)(),
Are you ever using this constructor?

https://codereview.chromium.org/1092353002/diff/240001/test/unittests/compile...
test/unittests/compiler/js-operator-unittest.cc:41: effect_input_count,
control_input_count,
Nit: weird indentation

https://codereview.chromium.org/1092353002/diff/240001/test/unittests/compile...
test/unittests/compiler/js-operator-unittest.cc:83:
SharedOperatorWithLanguageMode(const Operator* (JSOperatorBuilder::*cached)
Or this one?

https://codereview.chromium.org/1092353002/diff/240001/test/unittests/compile...
File test/unittests/compiler/typer-unittest.cc (right):

https://codereview.chromium.org/1092353002/diff/240001/test/unittests/compile...
test/unittests/compiler/typer-unittest.cc:360: V(LessThan)                      
  \
Nit: indentation

[Michael Starzinger, 2015-04-23 13:47:52]

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/ast-graph...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:1272: Node* exit_cond =
NewNode(javascript()->LessThan(language_mode()),
The index used here is guaranteed to be a Smi, so we can use SLOPPY here.

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:1307:
NewNode(javascript()->Add(language_mode()), index,
Likewise.

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:1332:
NewNode(javascript()->Add(language_mode()), index,
Likewise.

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/js-operat...
File src/compiler/js-operator.cc (right):

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/js-operat...
src/compiler/js-operator.cc:233: #define CACHED_OP_LIST_WITH_STRONG(V)          
          \
nit: s/WITH_STRONG/WITH_LANGUAGE_MODE/

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/js-typed-...
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/js-typed-...
src/compiler/js-typed-lowering.cc:347: return ReduceStrongNumberBinop(node,
numberOp);
On 2015/04/23 13:29:37, rossberg wrote:
> I'd just inline the function.

+1

[conradw, 2015-04-23 14:51:55]

https://codereview.chromium.org/1092353002/diff/240001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/1092353002/diff/240001/src/code-stubs.h#newco...
src/code-stubs.h:1253: BinaryOpICState state(isolate, op, language_mode);
On 2015/04/23 13:29:37, rossberg wrote:
> I think it's right for the BinoryOpICStub constructor to take the language
mode.
> However, BinaryOpICState should only take a bool, since otherwise we'd start
> distinguishing sloppy vs strict in the state and hence stub key, which implies
> that we'll cache them separately.

Done.

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/ast-graph...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:1272: Node* exit_cond =
NewNode(javascript()->LessThan(language_mode()),
On 2015/04/23 13:47:51, Michael Starzinger wrote:
> The index used here is guaranteed to be a Smi, so we can use SLOPPY here.

Done.

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/js-operat...
File src/compiler/js-operator.cc (right):

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/js-operat...
src/compiler/js-operator.cc:233: #define CACHED_OP_LIST_WITH_STRONG(V)          
          \
On 2015/04/23 13:47:51, Michael Starzinger wrote:
> nit: s/WITH_STRONG/WITH_LANGUAGE_MODE/

These operators are specifically associated with a macro which caches
exactly sloppy and strong versions. I wanted to avoid name oddness re.
other operators which might vary their semantics for different
combinations of language mode, and may in future get their own macros
(if StoreProperty were no longer the only operator to need a
sloppy/strict version, CACHED_OP_LIST_WITH_STRICT?)

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/js-typed-...
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/js-typed-...
src/compiler/js-typed-lowering.cc:347: return ReduceStrongNumberBinop(node,
numberOp);
On 2015/04/23 13:29:37, rossberg wrote:
> I'd just inline the function.

Done.

https://codereview.chromium.org/1092353002/diff/240001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/1092353002/diff/240001/src/ic/ic.cc#newcode2817
src/ic/ic.cc:2817: return is_strong(language_mode) ? Builtins::SUB_STRONG :
Builtins::SUB;
On 2015/04/23 13:29:37, rossberg wrote:
> Nit: considering that you'll have to add the same to every case eventually, it
> seems better to transpose the switch and conditional.
> 
> Btw, all the breaks in this switch can be removed.

Done.

https://codereview.chromium.org/1092353002/diff/240001/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1092353002/diff/240001/src/runtime.js#newcode202
src/runtime.js:202: //ECMA-262, section 11.6.2, page 50.
On 2015/04/23 13:29:37, rossberg wrote:
> Nit: spacing (here and below)

Done. It looks like this is a "feature" of Eclipse when copying comments.

https://codereview.chromium.org/1092353002/diff/240001/test/cctest/cctest.h
File test/cctest/cctest.h (right):

https://codereview.chromium.org/1092353002/diff/240001/test/cctest/cctest.h#n...
test/cctest/cctest.h:40: #ifndef TEST_WITH_STRONG
On 2015/04/23 13:29:37, rossberg wrote:
> I'd prefer to move this elsewhere, it is a bit too specialised for this
header.

Done.

https://codereview.chromium.org/1092353002/diff/240001/test/cctest/compiler/t...
File test/cctest/compiler/test-js-typed-lowering.cc (right):

https://codereview.chromium.org/1092353002/diff/240001/test/cctest/compiler/t...
test/cctest/compiler/test-js-typed-lowering.cc:1062: printf("%s\n%s\n", R.ops[j
+ 1]->mnemonic(), B.result->op()->mnemonic());
On 2015/04/23 13:29:37, rossberg wrote:
> Stray debug output?

Oops, done.

https://codereview.chromium.org/1092353002/diff/240001/test/mjsunit/strong/im...
File test/mjsunit/strong/implicit-casts.js (right):

https://codereview.chromium.org/1092353002/diff/240001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-casts.js:1: // Copyright 2015 the V8 project
authors. All rights reserved.
On 2015/04/23 13:29:37, rossberg wrote:
> Nit: rename file to implicit-conversions.js

Done.

https://codereview.chromium.org/1092353002/diff/240001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-casts.js:9: //TODO(conradw): Implement other strong
operators
On 2015/04/23 13:29:37, rossberg wrote:
> Nit: spacing

Done.

https://codereview.chromium.org/1092353002/diff/240001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-casts.js:17: let cast_values = [
On 2015/04/23 13:29:37, rossberg wrote:
> Nit: nonnumber_values

Done.

https://codereview.chromium.org/1092353002/diff/240001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-casts.js:27: let uncast_values = [
On 2015/04/23 13:29:37, rossberg wrote:
> Nit: number_values

Done.

https://codereview.chromium.org/1092353002/diff/240001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-casts.js:95: for (let v_left of uncast_values) {
On 2015/04/23 13:29:37, rossberg wrote:
> Nit: drop the v_

Done.

https://codereview.chromium.org/1092353002/diff/240001/test/unittests/compile...
File test/unittests/compiler/js-operator-unittest.cc (right):

https://codereview.chromium.org/1092353002/diff/240001/test/unittests/compile...
test/unittests/compiler/js-operator-unittest.cc:33: SharedOperator(const
Operator* (JSOperatorBuilder::*cached)(),
On 2015/04/23 13:29:37, rossberg wrote:
> Are you ever using this constructor?

Yes, my C++ is rusty about what exactly is going on, but the constructor is (and
was) invoked when the members of kSharedOperators[] are declared.

https://codereview.chromium.org/1092353002/diff/240001/test/unittests/compile...
test/unittests/compiler/js-operator-unittest.cc:41: effect_input_count,
control_input_count,
On 2015/04/23 13:29:37, rossberg wrote:
> Nit: weird indentation

Made a stab at it.

https://codereview.chromium.org/1092353002/diff/240001/test/unittests/compile...
test/unittests/compiler/js-operator-unittest.cc:83:
SharedOperatorWithLanguageMode(const Operator* (JSOperatorBuilder::*cached)
On 2015/04/23 13:29:37, rossberg wrote:
> Or this one?

Ditto for kSharedOperatorsWithlanguageMode[].

[Michael Starzinger, 2015-04-23 15:20:54]

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/js-operat...
File src/compiler/js-operator.cc (right):

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/js-operat...
src/compiler/js-operator.cc:233: #define CACHED_OP_LIST_WITH_STRONG(V)          
          \
On 2015/04/23 14:51:54, conradw wrote:
> On 2015/04/23 13:47:51, Michael Starzinger wrote:
> > nit: s/WITH_STRONG/WITH_LANGUAGE_MODE/
> 
> These operators are specifically associated with a macro which caches
> exactly sloppy and strong versions. I wanted to avoid name oddness re.
> other operators which might vary their semantics for different
> combinations of language mode, and may in future get their own macros
> (if StoreProperty were no longer the only operator to need a
> sloppy/strict version, CACHED_OP_LIST_WITH_STRICT?)

Yep, the implementation only provides caches for SLOPPY and STRONG, but the
interface is still providing the full language mode. As for the case that we
ever need another second order macro, we can revisit the naming then. I still
think CACHED_OP_LIST_WITH_LANGUAGE_MODE makes more sense because it best
describes the interface provided for those operators.

https://codereview.chromium.org/1092353002/diff/260001/src/compiler/js-operat...
File src/compiler/js-operator.h (right):

https://codereview.chromium.org/1092353002/diff/260001/src/compiler/js-operat...
src/compiler/js-operator.h:236: const Operator* UnaryNot(LanguageMode
language_mode);
The JSUnaryNot operator shouldn't require the language mode.

[rossberg, 2015-04-23 15:36:58]

https://codereview.chromium.org/1092353002/diff/240001/test/unittests/compile...
File test/unittests/compiler/js-operator-unittest.cc (right):

https://codereview.chromium.org/1092353002/diff/240001/test/unittests/compile...
test/unittests/compiler/js-operator-unittest.cc:33: SharedOperator(const
Operator* (JSOperatorBuilder::*cached)(),
On 2015/04/23 14:51:55, conradw wrote:
> On 2015/04/23 13:29:37, rossberg wrote:
> > Are you ever using this constructor?
> 
> Yes, my C++ is rusty about what exactly is going on, but the constructor is
(and
> was) invoked when the members of kSharedOperators[] are declared.

I think it would be much simpler to just parameterise SharedOperator over the
type of the constructor, and do away with all the inheritance.

[conradw, 2015-04-23 16:08:02]

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/js-operat...
File src/compiler/js-operator.cc (right):

https://codereview.chromium.org/1092353002/diff/240001/src/compiler/js-operat...
src/compiler/js-operator.cc:233: #define CACHED_OP_LIST_WITH_STRONG(V)          
          \
On 2015/04/23 15:20:53, Michael Starzinger wrote:
> On 2015/04/23 14:51:54, conradw wrote:
> > On 2015/04/23 13:47:51, Michael Starzinger wrote:
> > > nit: s/WITH_STRONG/WITH_LANGUAGE_MODE/
> > 
> > These operators are specifically associated with a macro which caches
> > exactly sloppy and strong versions. I wanted to avoid name oddness re.
> > other operators which might vary their semantics for different
> > combinations of language mode, and may in future get their own macros
> > (if StoreProperty were no longer the only operator to need a
> > sloppy/strict version, CACHED_OP_LIST_WITH_STRICT?)
> 
> Yep, the implementation only provides caches for SLOPPY and STRONG, but the
> interface is still providing the full language mode. As for the case that we
> ever need another second order macro, we can revisit the naming then. I still
> think CACHED_OP_LIST_WITH_LANGUAGE_MODE makes more sense because it best
> describes the interface provided for those operators.

I'd like to keep something in the code drawing attention to the fact that it's
sloppy/strong. What do you think of the next change?

https://codereview.chromium.org/1092353002/diff/240001/test/unittests/compile...
File test/unittests/compiler/js-operator-unittest.cc (right):

https://codereview.chromium.org/1092353002/diff/240001/test/unittests/compile...
test/unittests/compiler/js-operator-unittest.cc:33: SharedOperator(const
Operator* (JSOperatorBuilder::*cached)(),
On 2015/04/23 15:36:58, rossberg wrote:
> On 2015/04/23 14:51:55, conradw wrote:
> > On 2015/04/23 13:29:37, rossberg wrote:
> > > Are you ever using this constructor?
> > 
> > Yes, my C++ is rusty about what exactly is going on, but the constructor is
> (and
> > was) invoked when the members of kSharedOperators[] are declared.
> 
> I think it would be much simpler to just parameterise SharedOperator over the
> type of the constructor, and do away with all the inheritance.

Done. It's beautiful.

https://codereview.chromium.org/1092353002/diff/260001/src/compiler/js-operat...
File src/compiler/js-operator.h (right):

https://codereview.chromium.org/1092353002/diff/260001/src/compiler/js-operat...
src/compiler/js-operator.h:236: const Operator* UnaryNot(LanguageMode
language_mode);
On 2015/04/23 15:20:53, Michael Starzinger wrote:
> The JSUnaryNot operator shouldn't require the language mode.

I thought the semantics involved ToBoolean? Currently in my followup CL this
will be another possible throwing implicit conversion.

@rossberg?

[rossberg, 2015-04-24 09:50:21]

LGTM modulo nits

https://codereview.chromium.org/1092353002/diff/280001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/1092353002/diff/280001/src/ic/ic.cc#newcode2811
src/ic/ic.cc:2811: switch (op) {
Nit: to compress a little, can you put the returns on the same line as the
cases? Also, put the defaults last.

https://codereview.chromium.org/1092353002/diff/280001/test/unittests/compile...
File test/unittests/compiler/js-operator-unittest.cc (right):

https://codereview.chromium.org/1092353002/diff/280001/test/unittests/compile...
test/unittests/compiler/js-operator-unittest.cc:25: struct OperatorParams {
You should probably rename this back to SharedOperator now.

[conradw, 2015-04-24 10:55:16]

@mstarzinger: everything looks ok?

https://codereview.chromium.org/1092353002/diff/280001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/1092353002/diff/280001/src/ic/ic.cc#newcode2811
src/ic/ic.cc:2811: switch (op) {
On 2015/04/24 09:50:20, rossberg wrote:
> Nit: to compress a little, can you put the returns on the same line as the
> cases? Also, put the defaults last.

Sort of done, compressed the switch but putting the default at the end generates
"control may reach end of non-void function". I think this is the reason the
default case was at the top before I touched the code.

https://codereview.chromium.org/1092353002/diff/280001/test/unittests/compile...
File test/unittests/compiler/js-operator-unittest.cc (right):

https://codereview.chromium.org/1092353002/diff/280001/test/unittests/compile...
test/unittests/compiler/js-operator-unittest.cc:25: struct OperatorParams {
On 2015/04/24 09:50:20, rossberg wrote:
> You should probably rename this back to SharedOperator now.

Done.

[rossberg, 2015-04-24 11:08:18]

https://codereview.chromium.org/1092353002/diff/260001/src/compiler/js-operat...
File src/compiler/js-operator.h (right):

https://codereview.chromium.org/1092353002/diff/260001/src/compiler/js-operat...
src/compiler/js-operator.h:236: const Operator* UnaryNot(LanguageMode
language_mode);
On 2015/04/23 16:08:02, conradw wrote:
> On 2015/04/23 15:20:53, Michael Starzinger wrote:
> > The JSUnaryNot operator shouldn't require the language mode.
> 
> I thought the semantics involved ToBoolean? Currently in my followup CL this
> will be another possible throwing implicit conversion.
> 
> @rossberg?

ToBoolean is the one conversion that we will keep, so the mode is not needed
here.

[Michael Starzinger, 2015-04-24 11:26:38]

https://codereview.chromium.org/1092353002/diff/300001/src/compiler/js-operat...
File src/compiler/js-operator.h (right):

https://codereview.chromium.org/1092353002/diff/300001/src/compiler/js-operat...
src/compiler/js-operator.h:236: const Operator* UnaryNot(LanguageMode
language_mode);
Let me try to rephrase my original comment: Please remove the language mode
here, it is not needed.

[conradw, 2015-04-24 11:35:37]

https://codereview.chromium.org/1092353002/diff/260001/src/compiler/js-operat...
File src/compiler/js-operator.h (right):

https://codereview.chromium.org/1092353002/diff/260001/src/compiler/js-operat...
src/compiler/js-operator.h:236: const Operator* UnaryNot(LanguageMode
language_mode);
On 2015/04/24 11:08:18, rossberg wrote:
> On 2015/04/23 16:08:02, conradw wrote:
> > On 2015/04/23 15:20:53, Michael Starzinger wrote:
> > > The JSUnaryNot operator shouldn't require the language mode.
> > 
> > I thought the semantics involved ToBoolean? Currently in my followup CL this
> > will be another possible throwing implicit conversion.
> > 
> > @rossberg?
> 
> ToBoolean is the one conversion that we will keep, so the mode is not needed
> here.

Ah, I should have remembered. Done.

https://codereview.chromium.org/1092353002/diff/300001/src/compiler/js-operat...
File src/compiler/js-operator.h (right):

https://codereview.chromium.org/1092353002/diff/300001/src/compiler/js-operat...
src/compiler/js-operator.h:236: const Operator* UnaryNot(LanguageMode
language_mode);
On 2015/04/24 11:26:37, Michael Starzinger wrote:
> Let me try to rephrase my original comment: Please remove the language mode
> here, it is not needed.

Done, forgot ToBoolean would not cause a throw.

[Michael Starzinger, 2015-04-24 11:47:07]

LGTM. The CL description talks about "casts", I think it it would make more
sense to call it "conversion".

https://codereview.chromium.org/1092353002/diff/320001/test/cctest/compiler/t...
File test/cctest/compiler/test-js-typed-lowering.cc (right):

https://codereview.chromium.org/1092353002/diff/320001/test/cctest/compiler/t...
test/cctest/compiler/test-js-typed-lowering.cc:234: TEST(StringBinops) {
nit: s/TEST/TEST_WITH_STRONG/

https://codereview.chromium.org/1092353002/diff/320001/test/unittests/compile...
File test/unittests/compiler/js-typed-lowering-unittest.cc (right):

https://codereview.chromium.org/1092353002/diff/320001/test/unittests/compile...
test/unittests/compiler/js-typed-lowering-unittest.cc:117:
Reduce(graph()->NewNode(javascript()->UnaryNot(),
nit: Several whitespace changes here and throughout the file.

[conradw, 2015-04-24 12:04:44]

Renamed

https://codereview.chromium.org/1092353002/diff/320001/test/cctest/compiler/t...
File test/cctest/compiler/test-js-typed-lowering.cc (right):

https://codereview.chromium.org/1092353002/diff/320001/test/cctest/compiler/t...
test/cctest/compiler/test-js-typed-lowering.cc:234: TEST(StringBinops) {
On 2015/04/24 11:47:06, Michael Starzinger wrote:
> nit: s/TEST/TEST_WITH_STRONG/

Done.

https://codereview.chromium.org/1092353002/diff/320001/test/unittests/compile...
File test/unittests/compiler/js-typed-lowering-unittest.cc (right):

https://codereview.chromium.org/1092353002/diff/320001/test/unittests/compile...
test/unittests/compiler/js-typed-lowering-unittest.cc:117:
Reduce(graph()->NewNode(javascript()->UnaryNot(),
On 2015/04/24 11:47:06, Michael Starzinger wrote:
> nit: Several whitespace changes here and throughout the file.

Done.

[conradw, 2015-04-24 12:30:44]

The CQ bit was checked by conradw@chromium.org

[conradw, 2015-04-24 12:30:44]

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/1092353002/#ps340001
(title: "cl feedback 5")

[commit-bot: I haz the power, 2015-04-24 12:30:58]

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

[commit-bot: I haz the power, 2015-04-24 12:33:05]

Committed patchset #9 (id:340001)

[commit-bot: I haz the power, 2015-04-24 12:33:19]

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

[arv (Not doing code reviews), 2015-04-24 14:14:21]

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

[arv (Not doing code reviews), 2015-04-24 14:14:23]

LGTM

Since `git cl format` does not check JavaScript I used the arv linter instead.

https://codereview.chromium.org/1092353002/diff/340001/src/ic/ic-state.h
File src/ic/ic-state.h (right):

https://codereview.chromium.org/1092353002/diff/340001/src/ic/ic-state.h#newc...
src/ic/ic-state.h:151: class StrongField : public BitField<bool, 10, 1> {};
We should update these to use kNext when we make changes to them.

https://codereview.chromium.org/1092353002/diff/340001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/1092353002/diff/340001/src/ic/ic.cc#newcode2810
src/ic/ic.cc:2810: if (is_strong(language_mode)) {
Why is this needed? The two branches looks identical to me.

https://codereview.chromium.org/1092353002/diff/340001/test/mjsunit/strong/im...
File test/mjsunit/strong/implicit-conversions.js (right):

https://codereview.chromium.org/1092353002/diff/340001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-conversions.js:11: "-",
Array and Object literals should be indented 2 spaces and not 4.

https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...

https://codereview.chromium.org/1092353002/diff/340001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-conversions.js:11: "-",
Don't mix ' and " in the same file. ' is preferred:

https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...

If you modify an existing file use whatever is in there.

https://codereview.chromium.org/1092353002/diff/340001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-conversions.js:15: ]
semicolon

https://codereview.chromium.org/1092353002/diff/340001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-conversions.js:17: let nonnumber_values = [
nonNumberValues

https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...

https://codereview.chromium.org/1092353002/diff/340001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-conversions.js:51: return v;
why not just `return x - y`?

Is this to prevent inlining?

https://codereview.chromium.org/1092353002/diff/340001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-conversions.js:109: assertThrows("'use strong'; let
v = " + expr + ";", TypeError);
These things might be easier to read with template literals?

https://codereview.chromium.org/1092353002/diff/340001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-conversions.js:146: } catch(e) {
catch (e)

Just like if, for etc

https://codereview.chromium.org/1092353002/diff/340001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-conversions.js:147: assertTrue(e instanceof
TypeError);
or assertInstanceof

https://codereview.chromium.org/1092353002/diff/340001/test/unittests/compile...
File test/unittests/compiler/js-operator-unittest.cc (right):

https://codereview.chromium.org/1092353002/diff/340001/test/unittests/compile...
test/unittests/compiler/js-operator-unittest.cc:74:
kSharedOperatorsWithlanguageMode[] = {
strange indentation

[conradw, 2015-04-24 14:42:32]

https://codereview.chromium.org/1092353002/diff/340001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/1092353002/diff/340001/src/ic/ic.cc#newcode2810
src/ic/ic.cc:2810: if (is_strong(language_mode)) {
On 2015/04/24 14:14:23, arv wrote:
> Why is this needed? The two branches looks identical to me.

In strong mode, the strong version of the builtin needs to be selected (e.g.
Builtins::SUB_STRONG). Eventually, all of the cases in the strong branch will
select strong versions, but some ops are still WIP.

https://codereview.chromium.org/1092353002/diff/340001/test/mjsunit/strong/im...
File test/mjsunit/strong/implicit-conversions.js (right):

https://codereview.chromium.org/1092353002/diff/340001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-conversions.js:11: "-",
On 2015/04/24 14:14:23, arv wrote:
> Don't mix ' and " in the same file. ' is preferred:
> 
>
https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...
> 
> If you modify an existing file use whatever is in there.

I've been trying to be consistent in using ", which seems to be preferred by the
other strong mode tests. ' has been used to mark out string literals when the
"outer string" is passed into an eval-like function.

https://codereview.chromium.org/1092353002/diff/340001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-conversions.js:11: "-",
On 2015/04/24 14:14:23, arv wrote:
> Array and Object literals should be indented 2 spaces and not 4.
> 
>
https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...

Will add to my followup https://codereview.chromium.org/1102923002/

https://codereview.chromium.org/1092353002/diff/340001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-conversions.js:15: ]
On 2015/04/24 14:14:23, arv wrote:
> semicolon

followup

https://codereview.chromium.org/1092353002/diff/340001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-conversions.js:17: let nonnumber_values = [
On 2015/04/24 14:14:23, arv wrote:
> nonNumberValues
> 
>
https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...

followup

https://codereview.chromium.org/1092353002/diff/340001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-conversions.js:51: return v;
On 2015/04/24 14:14:23, arv wrote:
> why not just `return x - y`?
> 
> Is this to prevent inlining?

there's no particular reason for it, will change in followup

https://codereview.chromium.org/1092353002/diff/340001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-conversions.js:109: assertThrows("'use strong'; let
v = " + expr + ";", TypeError);
On 2015/04/24 14:14:23, arv wrote:
> These things might be easier to read with template literals?

Probably. It doesn't sound like a bad idea for me to change the strong tests
built like this to use template literals sometime. There are a decent number.

https://codereview.chromium.org/1092353002/diff/340001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-conversions.js:146: } catch(e) {
On 2015/04/24 14:14:23, arv wrote:
> catch (e)
> 
> Just like if, for etc

followup

https://codereview.chromium.org/1092353002/diff/340001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-conversions.js:147: assertTrue(e instanceof
TypeError);
On 2015/04/24 14:14:23, arv wrote:
> or assertInstanceof

followup

https://codereview.chromium.org/1092353002/diff/340001/test/unittests/compile...
File test/unittests/compiler/js-operator-unittest.cc (right):

https://codereview.chromium.org/1092353002/diff/340001/test/unittests/compile...
test/unittests/compiler/js-operator-unittest.cc:74:
kSharedOperatorsWithlanguageMode[] = {
On 2015/04/24 14:14:23, arv wrote:
> strange indentation

I guess I was slightly religious in sticking to the format of the existing
macro. In fact now that I think about it it's intensely silly that I
define-undef-define-undef this macro, both uses are identical.