[Interpreter] Adds delete operator to interpreter.

src/interpreter/bytecode-generator.cc

Index: src/interpreter/bytecode-generator.cc
diff --git a/src/interpreter/bytecode-generator.cc b/src/interpreter/bytecode-generator.cc
index 309b9294297674b1b6ff75e4214df11865624620..6102231f32a9d06725d9e5aef40612dc1edd1fe0 100644
--- a/src/interpreter/bytecode-generator.cc
+++ b/src/interpreter/bytecode-generator.cc
@@ -1146,8 +1146,14 @@ void BytecodeGenerator::VisitUnaryOperation(UnaryOperation* expr) {
     case Token::Value::VOID:
       VisitVoid(expr);
       break;
-    case Token::Value::BIT_NOT:
     case Token::Value::DELETE:
+      VisitDeleteOperation(expr);
+      break;
+    // These operators are converted to an equivalent binary operators in
+    // the parser. These operators are not expected to be visited here.
+    case Token::Value::BIT_NOT:
+    case Token::Value::ADD:
+    case Token::Value::SUB:
       UNIMPLEMENTED();

[rmcilroy, 2015/10/21 14:52:54]

These should be UNREACHABLE instead of UNIMPLEMENTED if we don't expect to reach
them. Move the comment into the case body.

     default:
       UNREACHABLE();
@@ -1155,6 +1161,60 @@ void BytecodeGenerator::VisitUnaryOperation(UnaryOperation* expr) {
 }
 
 
+void BytecodeGenerator::VisitDeleteOperation(UnaryOperation* expr) {

[rmcilroy, 2015/10/21 14:52:54]

nit - drop Operator (just VisitDelete)

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

Done.

+  if (expr->expression()->IsProperty()) {
+    Property* property = expr->expression()->AsProperty();
+    TemporaryRegisterScope temporary_register_scope(builder());
+    Register temporary = temporary_register_scope.NewRegister();

[rmcilroy, 2015/10/21 14:52:54]

call this object (no need to store property->obj() or property->key() in local
variables, just pass them directly to the Visit function calls)

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

Done.

+    Expression* obj = property->obj();
+    Visit(obj);
+    builder()->StoreAccumulatorInRegister(temporary);
+    Expression* key = property->key();
+    Visit(key);
+    builder()->Delete(temporary, language_mode());
+  } else if (expr->expression()->IsVariableProxy()) {
+    VariableProxy* proxy_expr = expr->expression()->AsVariableProxy();
+    VisitVariableDelete(proxy_expr);

[rmcilroy, 2015/10/21 14:52:54]

nit - I would just inline this since it isn't called anywhere else.

[mythria, 2015/10/23 14:48:00]

Done.

+  } else {
+    Visit(expr->expression());

[rmcilroy, 2015/10/21 14:52:54]

Comments please (throughout this whole function and VisitVariableDelete)
explaining the different situations (similar to the comments in full-codegen and
ast-graph-builder)

[mythria, 2015/10/23 14:48:00]

Done.

+    builder()->LoadTrue();
+  }
+}
+
+
+void BytecodeGenerator::VisitVariableDelete(VariableProxy* proxy) {
+  Variable* variable = proxy->var();
+  DCHECK(is_sloppy(language_mode()) || variable->HasThisName(isolate()));
+  switch (variable->location()) {
+    case VariableLocation::GLOBAL:
+    case VariableLocation::UNALLOCATED: {
+      TemporaryRegisterScope temporary_register_scope(builder());
+      Register temporary = temporary_register_scope.NewRegister();

[rmcilroy, 2015/10/21 14:52:54]

nit - call this global_object

[mythria, 2015/10/23 14:48:00]

Done.

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

[rmcilroy, 2015/10/21 14:52:54]

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());

[mythria, 2015/10/23 14:48:00]

Done.

+    }
+      return;

[rmcilroy, 2015/10/21 14:52:54]

break, not return, and put it inside the "{ .. }" block scope.

[mythria, 2015/10/23 14:48:00]

Done.

+    case VariableLocation::PARAMETER:
+    case VariableLocation::LOCAL:
+    case VariableLocation::CONTEXT: {
+      if (variable->HasThisName(isolate())) {

[rmcilroy, 2015/10/21 14:52:54]

comments

[mythria, 2015/10/23 14:48:00]

Done.

+        builder()->LoadTrue();
+      } else {
+        builder()->LoadFalse();
+      }
+    }
+      return;

[rmcilroy, 2015/10/21 14:52:54]

ditto

[mythria, 2015/10/23 14:48:00]

Done.

+    case VariableLocation::LOOKUP: {
+      UNIMPLEMENTED();

[rmcilroy, 2015/10/21 14:52:54]

break;

[mythria, 2015/10/23 14:48:00]

Done.

+    }
+      UNREACHABLE();

[rmcilroy, 2015/10/21 14:52:54]

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)

[mythria, 2015/10/23 14:48:00]

Done.

+  }
+}
+
+
 void BytecodeGenerator::VisitCountOperation(CountOperation* expr) {
   UNIMPLEMENTED();
 }