[Interpreter] Adds delete operator to interpreter.

src/interpreter/bytecode-generator.cc

 // Copyright 2015 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/interpreter/bytecode-generator.h"
 
 #include <stack>
 
 #include "src/compiler.h"
 #include "src/interpreter/control-flow-builders.h"
@@ skipping 1128 matching lines @@
   switch (expr->op()) {
     case Token::Value::NOT:
       VisitNot(expr);
       break;
     case Token::Value::TYPEOF:
       VisitTypeOf(expr);
       break;
     case Token::Value::VOID:
       VisitVoid(expr);
       break;
+    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::DELETE:
+    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();
   }
 }
 
 
+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();
 }
 
 
 void BytecodeGenerator::VisitBinaryOperation(BinaryOperation* binop) {
   switch (binop->op()) {
     case Token::COMMA:
       VisitCommaExpression(binop);
       break;
@@ skipping 253 matching lines @@
 }
 
 
 int BytecodeGenerator::feedback_index(FeedbackVectorSlot slot) const {
   return info()->feedback_vector()->GetIndex(slot);
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8