ParameterInitializerRewriter must maintain temporary variable order

src/ast/scopes.cc

 // Copyright 2012 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/ast/scopes.h"
 
 #include "src/accessors.h"
 #include "src/ast/scopeinfo.h"
 #include "src/bootstrapper.h"
 #include "src/messages.h"
@@ skipping 540 matching lines @@
   Scope* scope = this->ClosureScope();
   Variable* var = new(zone()) Variable(scope,
                                        name,
                                        TEMPORARY,
                                        Variable::NORMAL,
                                        kCreatedInitialized);
   scope->AddTemporary(var);
   return var;
 }
 
-
-bool Scope::RemoveTemporary(Variable* var) {
+int Scope::RemoveTemporary(Variable* var) {
+  DCHECK_NOT_NULL(var);
   // Most likely (always?) any temporary variable we want to remove
   // was just added before, so we search backwards.
   for (int i = temps_.length(); i-- > 0;) {
     if (temps_[i] == var) {
-      temps_.Remove(i);
-      return true;
+      // Don't shrink temps_, as callers of this method expect
+      // the returned indices to be unique per-scope.
+      temps_[i] = nullptr;
+      return i;
     }
   }
-  return false;
+  return -1;
 }
 
 
 void Scope::AddDeclaration(Declaration* declaration) {
   decls_.Add(declaration, zone());
 }
 
 
 Declaration* Scope::CheckConflictingVarDeclarations() {
   int length = decls_.length();
@@ skipping 46 matching lines @@
                                          ZoneList<Variable*>* context_locals,
                                          ZoneList<Variable*>* context_globals) {
   DCHECK(stack_locals != NULL);
   DCHECK(context_locals != NULL);
   DCHECK(context_globals != NULL);
 
   // Collect temporaries which are always allocated on the stack, unless the
   // context as a whole has forced context allocation.
   for (int i = 0; i < temps_.length(); i++) {
     Variable* var = temps_[i];
+    if (var == nullptr) continue;
     if (var->is_used()) {
       if (var->IsContextSlot()) {
         DCHECK(has_forced_context_allocation());
         context_locals->Add(var, zone());
       } else if (var->IsStackLocal()) {
         stack_locals->Add(var, zone());
       } else {
         DCHECK(var->IsParameter());
       }
     }
@@ skipping 328 matching lines @@
     PrintF("%d heap slots (including %d global slots)\n", num_heap_slots_,
            num_global_slots_);
   }
 
   // Print locals.
   if (function_ != NULL) {
     Indent(n1, "// function var:\n");
     PrintVar(n1, function_->proxy()->var());
   }
 
   if (temps_.length() > 0) {

[nickie, 2016/05/10 13:47:08]

If you have length() > 0 but only null elements, you'll still print the header.

[adamk, 2016/05/10 17:36:11]

This is probably the ugliest part of this approach :)

Fixed.

     Indent(n1, "// temporary vars:\n");
     for (int i = 0; i < temps_.length(); i++) {
-      PrintVar(n1, temps_[i]);
+      if (temps_[i] != NULL) {

[nickie, 2016/05/10 13:47:08]

Not nullptr?  :-)

[adamk, 2016/05/10 17:36:11]

Was going for consistent, but looks like this function is already inconsistent.
Switched :)

+        PrintVar(n1, temps_[i]);
+      }
     }
   }
 
   if (variables_.Start() != NULL) {
     Indent(n1, "// local vars:\n");
     PrintMap(n1, &variables_);
   }
 
   if (dynamics_ != NULL) {
     Indent(n1, "// dynamic vars:\n");
@@ skipping 409 matching lines @@
       // There must be only DYNAMIC_GLOBAL in the script scope.
       DCHECK(!is_script_scope() || DYNAMIC_GLOBAL == var->mode());
     }
   }
 }
 
 
 void Scope::AllocateNonParameterLocalsAndDeclaredGlobals(Isolate* isolate) {
   // All variables that have no rewrite yet are non-parameter locals.
   for (int i = 0; i < temps_.length(); i++) {
+    if (temps_[i] == nullptr) continue;
     AllocateNonParameterLocal(isolate, temps_[i]);
   }
 
   ZoneList<VarAndOrder> vars(variables_.occupancy(), zone());
   for (VariableMap::Entry* p = variables_.Start();
        p != NULL;
        p = variables_.Next(p)) {
     Variable* var = reinterpret_cast<Variable*>(p->value);
     vars.Add(VarAndOrder(var, p->order), zone());
   }
@@ skipping 88 matching lines @@
       function_ != NULL && function_->proxy()->var()->IsContextSlot();
   return num_heap_slots() - Context::MIN_CONTEXT_SLOTS - num_global_slots() -
          (is_function_var_in_context ? 1 : 0);
 }
 
 
 int Scope::ContextGlobalCount() const { return num_global_slots(); }
 
 }  // namespace internal
 }  // namespace v8