[Interpreter] Adds delete operator to interpreter.

[Interpreter] Adds delete operator to interpreter.

Adds support for delete operator, it's implementation and tests.

Adds tests for the following unary operators
  -BitwiseNot
  -Add
  -Sub

BUG=v8:4280
LOG=N

Committed: https://crrev.com/95e26ec423fa5ac6375dc6950e8c31d494aba334
Cr-Commit-Position: refs/heads/master@{#31620}

[mythria, 2015-10-21 12:20:49]

mythria@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[mythria, 2015-10-21 12:20:50]

Hi,

I added support for delete operator. Could you please review the changes. I will
send it to compiler team for review after first few rounds with you.

Thanks and Regards,
Mythri

[rmcilroy, 2015-10-21 12:28:13]

Description was changed from

==========
[Interpreter] Adds delete operator to interpreter.

Adds support for delete operator, it's implementation and tests.
Adds tests for the follwoing unary operators
-BitwiseNot
-Add
-Sub

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Adds delete operator to interpreter.

Adds support for delete operator, it's implementation and tests.
Adds tests for the following unary operators
-BitwiseNot
-Add
-Sub

BUG=v8:4280
LOG=N
==========

[rmcilroy, 2015-10-21 12:28:20]

Description was changed from

==========
[Interpreter] Adds delete operator to interpreter.

Adds support for delete operator, it's implementation and tests.
Adds tests for the following unary operators
-BitwiseNot
-Add
-Sub

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Adds delete operator to interpreter.

Adds support for delete operator, it's implementation and tests.
Adds tests for the following unary operators
  -BitwiseNot
  -Add
  -Sub
 
BUG=v8:4280
LOG=N
==========

[rmcilroy, 2015-10-21 12:28:25]

Description was changed from

==========
[Interpreter] Adds delete operator to interpreter.

Adds support for delete operator, it's implementation and tests.
Adds tests for the following unary operators
  -BitwiseNot
  -Add
  -Sub
 
BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Adds delete operator to interpreter.

Adds support for delete operator, it's implementation and tests.

Adds tests for the following unary operators
  -BitwiseNot
  -Add
  -Sub
 
BUG=v8:4280
LOG=N
==========

[rmcilroy, 2015-10-21 14:52:55]

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:638: Bytecode bytecode =
(language_mode == STRICT)
Could you move this code into a BytecodeForDelete function similar to
BytecodeForStoreGlobal, with a switch on language_mode and an explicitly
UNIMPLEMENTED for STRONG and UNREACHABLE for default.

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.h:138: BytecodeArrayBuilder&
Delete(Register object, LanguageMode language_mode);
add a comment saying the "key" is expected to be in the accumulator

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1157: UNIMPLEMENTED();
These should be UNREACHABLE instead of UNIMPLEMENTED if we don't expect to reach
them. Move the comment into the case body.

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1164: void
BytecodeGenerator::VisitDeleteOperation(UnaryOperation* expr) {
nit - drop Operator (just VisitDelete)

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1168: Register temporary =
temporary_register_scope.NewRegister();
call this object (no need to store property->obj() or property->key() in local
variables, just pass them directly to the Visit function calls)

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1177: VisitVariableDelete(proxy_expr);
nit - I would just inline this since it isn't called anywhere else.

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1179: Visit(expr->expression());
Comments please (throughout this whole function and VisitVariableDelete)
explaining the different situations (similar to the comments in full-codegen and
ast-graph-builder)

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1192: Register temporary =
temporary_register_scope.NewRegister();
nit - call this global_object

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1197: builder()->Delete(temporary,
language_mode());
nit - chain the builder calls - i.e.: 

builder()
    ->LoadContextSlot(execution_context()->reg(), Context::GLOBAL_OBJECT_INDEX)
    .StoreAccumulatorInRegister(global_object)
    .LoadLiteral(variable->name())
    .Delete(temporary, language_mode());

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1199: return;
break, not return, and put it inside the "{ .. }" block scope.

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1203: if
(variable->HasThisName(isolate())) {
comments

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1209: return;
ditto

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1211: UNIMPLEMENTED();
break;

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1213: UNREACHABLE();
Remove this (if you don't have "default" then it is a compiler error if there is
a missing case, so no need for UNREACHABLE)

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:541: DoBinaryOp(Runtime::kDeleteProperty_Strict,
assembler);
Hmm, this isn't really a binary op, and when binary ops do more smarts than just
calling the runtime then this will be wrong. Could you just add a DoDelete(...)
which does the same thing and call it from both of these please.

https://codereview.chromium.org/1410953003/diff/1/test/cctest/interpreter/tes...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1410953003/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-bytecode-generator.cc:1895: ExpectedSnippet<int>
snippets[] = {{"var x = 0;"
Could you restore this to the previous indenting please - it's difficult to work
out what's changed here (and the new indenting is weird.. :).

https://codereview.chromium.org/1410953003/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-bytecode-generator.cc:2060: {"var a = {x:13, y:14};
return delete a.x;",
Could you add tests for the different types of delete operations please (named
property, keyed property, global, context allocated, local variable, etc.) i.e.,
similar to the tests for compound expression in
https://codereview.chromium.org/1420503002/.

https://codereview.chromium.org/1410953003/diff/1/test/cctest/interpreter/tes...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1410953003/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-interpreter.cc:2073: std::pair<const char*,
Handle<Object>> test_global_delete[] = {
Globals are variables, not properties. Personally I wouldn't split this way - I
would do all the non-globals in one test (InterpreterDelete) and have a
different test for globals (InterpreterGlobalDelete) since globals can't use
InterpreterTester::SourceForBody(...). Again, you could follow the pattern in
https://codereview.chromium.org/1420503002/

https://codereview.chromium.org/1410953003/diff/1/test/unittests/interpreter/...
File test/unittests/interpreter/bytecode-array-builder-unittest.cc (right):

https://codereview.chromium.org/1410953003/diff/1/test/unittests/interpreter/...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:99: .Delete(reg,
LanguageMode::SLOPPY)
nit - make this a different section (since the bytecodes aren't really unary
operators even athough the delete operator is.

[mythria, 2015-10-23 14:48:01]

Can you please review my changes. I am not sure it is easy to see the diff after
re-basing. I can upload a new cl if you think it is easier that way. 

Thanks and Regards,
Mythri

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:638: Bytecode bytecode =
(language_mode == STRICT)
On 2015/10/21 14:52:54, rmcilroy wrote:
> Could you move this code into a BytecodeForDelete function similar to
> BytecodeForStoreGlobal, with a switch on language_mode and an explicitly
> UNIMPLEMENTED for STRONG and UNREACHABLE for default.

Done.

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.h:138: BytecodeArrayBuilder&
Delete(Register object, LanguageMode language_mode);
On 2015/10/21 14:52:54, rmcilroy wrote:
> add a comment saying the "key" is expected to be in the accumulator

Done.

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1164: void
BytecodeGenerator::VisitDeleteOperation(UnaryOperation* expr) {
On 2015/10/21 14:52:54, rmcilroy wrote:
> nit - drop Operator (just VisitDelete)

Done.

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1168: Register temporary =
temporary_register_scope.NewRegister();
On 2015/10/21 14:52:54, rmcilroy wrote:
> call this object (no need to store property->obj() or property->key() in local
> variables, just pass them directly to the Visit function calls)

Done.

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1177: VisitVariableDelete(proxy_expr);
On 2015/10/21 14:52:54, rmcilroy wrote:
> nit - I would just inline this since it isn't called anywhere else.

Done.

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1179: Visit(expr->expression());
On 2015/10/21 14:52:54, rmcilroy wrote:
> Comments please (throughout this whole function and VisitVariableDelete)
> explaining the different situations (similar to the comments in full-codegen
and
> ast-graph-builder)

Done.

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1192: Register temporary =
temporary_register_scope.NewRegister();
On 2015/10/21 14:52:54, rmcilroy wrote:
> nit - call this global_object

Done.

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1197: builder()->Delete(temporary,
language_mode());
On 2015/10/21 14:52:54, rmcilroy wrote:
> nit - chain the builder calls - i.e.: 
> 
> builder()
>     ->LoadContextSlot(execution_context()->reg(),
Context::GLOBAL_OBJECT_INDEX)
>     .StoreAccumulatorInRegister(global_object)
>     .LoadLiteral(variable->name())
>     .Delete(temporary, language_mode());

Done.

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1199: return;
On 2015/10/21 14:52:54, rmcilroy wrote:
> break, not return, and put it inside the "{ .. }" block scope.

Done.

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1203: if
(variable->HasThisName(isolate())) {
On 2015/10/21 14:52:54, rmcilroy wrote:
> comments

Done.

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1209: return;
On 2015/10/21 14:52:54, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1211: UNIMPLEMENTED();
On 2015/10/21 14:52:54, rmcilroy wrote:
> break;

Done.

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1213: UNREACHABLE();
On 2015/10/21 14:52:54, rmcilroy wrote:
> Remove this (if you don't have "default" then it is a compiler error if there
is
> a missing case, so no need for UNREACHABLE)

Done.

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1410953003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:541: DoBinaryOp(Runtime::kDeleteProperty_Strict,
assembler);
On 2015/10/21 14:52:54, rmcilroy wrote:
> Hmm, this isn't really a binary op, and when binary ops do more smarts than
just
> calling the runtime then this will be wrong. Could you just add a
DoDelete(...)
> which does the same thing and call it from both of these please.

Done.

https://codereview.chromium.org/1410953003/diff/1/test/cctest/interpreter/tes...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1410953003/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-bytecode-generator.cc:1895: ExpectedSnippet<int>
snippets[] = {{"var x = 0;"
On 2015/10/21 14:52:54, rmcilroy wrote:
> Could you restore this to the previous indenting please - it's difficult to
work
> out what's changed here (and the new indenting is weird.. :).

Done.

https://codereview.chromium.org/1410953003/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-bytecode-generator.cc:2060: {"var a = {x:13, y:14};
return delete a.x;",
On 2015/10/21 14:52:54, rmcilroy wrote:
> Could you add tests for the different types of delete operations please (named
> property, keyed property, global, context allocated, local variable, etc.)
i.e.,
> similar to the tests for compound expression in
> https://codereview.chromium.org/1420503002/.

Done.

https://codereview.chromium.org/1410953003/diff/1/test/cctest/interpreter/tes...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1410953003/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-interpreter.cc:2073: std::pair<const char*,
Handle<Object>> test_global_delete[] = {
On 2015/10/21 14:52:54, rmcilroy wrote:
> Globals are variables, not properties. Personally I wouldn't split this way -
I
> would do all the non-globals in one test (InterpreterDelete) and have a
> different test for globals (InterpreterGlobalDelete) since globals can't use
> InterpreterTester::SourceForBody(...). Again, you could follow the pattern in
> https://codereview.chromium.org/1420503002/

Done.

https://codereview.chromium.org/1410953003/diff/1/test/unittests/interpreter/...
File test/unittests/interpreter/bytecode-array-builder-unittest.cc (right):

https://codereview.chromium.org/1410953003/diff/1/test/unittests/interpreter/...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:99: .Delete(reg,
LanguageMode::SLOPPY)
On 2015/10/21 14:52:54, rmcilroy wrote:
> nit - make this a different section (since the bytecodes aren't really unary
> operators even athough the delete operator is.

Done.

[rmcilroy, 2015-10-26 14:31:07]

Looks good overall, a bunch of comments, but most are just nits.

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1385:
execution_result()->SetResultInAccumulator();
nit - do this once at the bottom of the function (since all code paths cause the
result to be in the accumulator)

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1395: // Global var, let, const or
variables not explicitly declared
nit - full stop at end of sentence

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1397: Register global_object =
temporary_register_scope.NewRegister();
nit - use exeuction_result()->NewRegister() (and remove TemporaryRegisterScope)

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1399: Context::GLOBAL_OBJECT_INDEX)
nit - indentation (did you run git cl format ? )

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1409: // Deleting Local var/let/const,
context variables, and arguments
nit - Local/local

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:497: compiler::InterpreterAssembler* assembler) {
indentation

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:504: }
nit - move this to be directly above DeletePropertyStrict

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.h (right):

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.h:89: // on the language mode.
nit - I don't think "function_id depends on the language mode." is required

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.h:91: compiler::InterpreterAssembler* assembler);
indentation

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:1885: TEST(Delete) {
Could you also make have a TEST(GlobalDelete) with a test or two for deleting
global objects (i.e., similar to GlobalCountOperators)

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:1977: };
nit - could you also test your unresolvable reference code path (i.e., "delete
'test';"

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:2298: 
nit - could you add a test for:
 - return delete this;
 - return delete "test";

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:2324: TEST(InterpreterDeleteSloppy)
{
nit - How about InterpreterDeleteSloppyUnqualifiedIdentifier to make what you
are testing explicit (since you are testing other sloppy mode stuff in the test
above).

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:2334: "a.x = a.x + !b;\n"
this is a bit hard to parse, how about:
var a = { x:10, y:'abc'};
if (delete a) {
  return undefined;
} else {
  return a.x;
}

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:2371: "  return a.x;}\n"
nit - '}' on a newline (throughout)

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:2380: std::make_pair("e = { x:10,
y:'abc'};\n"
nit - space after 'abc'

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:2388: "  var b = delete g;\n"
nit - just "return delete g;" to match the one above

[mythria, 2015-10-27 10:50:48]

Could you review my changes.

Thanks and Regards,
Mythri

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1385:
execution_result()->SetResultInAccumulator();
On 2015/10/26 14:31:06, rmcilroy wrote:
> nit - do this once at the bottom of the function (since all code paths cause
the
> result to be in the accumulator)

Done.

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1395: // Global var, let, const or
variables not explicitly declared
On 2015/10/26 14:31:06, rmcilroy wrote:
> nit - full stop at end of sentence

Done.

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1397: Register global_object =
temporary_register_scope.NewRegister();
On 2015/10/26 14:31:07, rmcilroy wrote:
> nit - use exeuction_result()->NewRegister() (and remove
TemporaryRegisterScope)

Done.

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1399: Context::GLOBAL_OBJECT_INDEX)
On 2015/10/26 14:31:07, rmcilroy wrote:
> nit - indentation (did you run git cl format ? )

Done.

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1409: // Deleting Local var/let/const,
context variables, and arguments
On 2015/10/26 14:31:06, rmcilroy wrote:
> nit - Local/local

Done.

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:497: compiler::InterpreterAssembler* assembler) {
On 2015/10/26 14:31:07, rmcilroy wrote:
> indentation

Done.

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:504: }
On 2015/10/26 14:31:07, rmcilroy wrote:
> nit - move this to be directly above DeletePropertyStrict

Done.

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.h (right):

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.h:89: // on the language mode.
On 2015/10/26 14:31:07, rmcilroy wrote:
> nit - I don't think "function_id depends on the language mode." is required

Done.

https://codereview.chromium.org/1410953003/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.h:91: compiler::InterpreterAssembler* assembler);
On 2015/10/26 14:31:07, rmcilroy wrote:
> indentation

Done.

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:1885: TEST(Delete) {
On 2015/10/26 14:31:07, rmcilroy wrote:
> Could you also make have a TEST(GlobalDelete) with a test or two for deleting
> global objects (i.e., similar to GlobalCountOperators) 

Done.

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:1977: };
On 2015/10/26 14:31:07, rmcilroy wrote:
> nit - could you also test your unresolvable reference code path (i.e., "delete
> 'test';" 

Done.

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:2298: 
On 2015/10/26 14:31:07, rmcilroy wrote:
> nit - could you add a test for:
>  - return delete this;
>  - return delete "test";
> 
>  

Done.

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:2324: TEST(InterpreterDeleteSloppy)
{
On 2015/10/26 14:31:07, rmcilroy wrote:
> nit - How about InterpreterDeleteSloppyUnqualifiedIdentifier to make what you
> are testing explicit (since you are testing other sloppy mode stuff in the
test
> above).

Done.

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:2334: "a.x = a.x + !b;\n"
On 2015/10/26 14:31:07, rmcilroy wrote:
> this is a bit hard to parse, how about:
> var a = { x:10, y:'abc'};
> if (delete a) {
>   return undefined;
> } else {
>   return a.x;
> }

Done.

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:2371: "  return a.x;}\n"
On 2015/10/26 14:31:07, rmcilroy wrote:
> nit - '}' on a newline (throughout)

Done.

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:2380: std::make_pair("e = { x:10,
y:'abc'};\n"
On 2015/10/26 14:31:07, rmcilroy wrote:
> nit - space after 'abc'

Done.

https://codereview.chromium.org/1410953003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:2388: "  var b = delete g;\n"
On 2015/10/26 14:31:07, rmcilroy wrote:
> nit - just "return delete g;" to match the one above

Done.

[rmcilroy, 2015-10-27 12:09:18]

LGTM, thanks!

[mythria, 2015-10-27 13:02:04]

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

[mythria, 2015-10-27 13:02:05]

Could you please review my changes. This cl adds support for delete operator and
also tests. 

Thanks and Regards,
Mythri

[Benedikt Meurer, 2015-10-28 05:00:09]

LGTM

[mythria, 2015-10-28 09:48:17]

The CQ bit was checked by mythria@chromium.org

[mythria, 2015-10-28 09:48:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1410953003/#ps60001
(title: "rebased the patch")

[commit-bot: I haz the power, 2015-10-28 09:48:24]

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

[commit-bot: I haz the power, 2015-10-28 09:49:26]

Committed patchset #4 (id:60001)

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

Patchset 4 (id:??) landed as
https://crrev.com/95e26ec423fa5ac6375dc6950e8c31d494aba334
Cr-Commit-Position: refs/heads/master@{#31620}