[parser] Avoid reversing inner PreparseScopeData order

Drive-by-fixes:
- Rename RestoreData to RestoreDataForScope
- Move Scope's private variable declaration to after functions

Change-Id: I349b2fc05cd94d6d7a0745b28b4e66b898b5e147
Reviewed-on: https://chromium-review.googlesource.com/c/1358517Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58010}

src/ast/scopes.cc

@@ -1512,9 +1512,8 @@ void Scope::SavePreParsedScopeData() {
 }
 
 void DeclarationScope::SavePreParsedScopeDataForDeclarationScope() {
-  if (preparsed_scope_data_builder_ != nullptr) {
-    preparsed_scope_data_builder_->SaveScopeAllocationData(this);
-  }
+  if (preparsed_scope_data_builder_ == nullptr) return;
+  preparsed_scope_data_builder_->SaveScopeAllocationData(this);
 }
 
 void DeclarationScope::AnalyzePartially(AstNodeFactory* ast_node_factory) {

src/ast/scopes.h

@@ -333,9 +333,7 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
     inner_scope_calls_eval_ = true;
     for (Scope* scope = outer_scope(); scope != nullptr;
          scope = scope->outer_scope()) {
-      if (scope->inner_scope_calls_eval_) {
-        return;
-      }
+      if (scope->inner_scope_calls_eval_) return;
       scope->inner_scope_calls_eval_ = true;
     }
   }
@@ -590,77 +588,6 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   // SavePreParsedScopeDataForDeclarationScope for each.
   void SavePreParsedScopeData();
 
-  Zone* zone_;
-
-  // Scope tree.
-  Scope* outer_scope_;  // the immediately enclosing outer scope, or nullptr
-  Scope* inner_scope_;  // an inner scope of this scope
-  Scope* sibling_;  // a sibling inner scope of the outer scope of this scope.
-
-  // The variables declared in this scope:
-  //
-  // All user-declared variables (incl. parameters).  For script scopes
-  // variables may be implicitly 'declared' by being used (possibly in
-  // an inner scope) with no intervening with statements or eval calls.
-  VariableMap variables_;
-  // In case of non-scopeinfo-backed scopes, this contains the variables of the
-  // map above in order of addition.
-  base::ThreadedList<Variable> locals_;
-  // Unresolved variables referred to from this scope. The proxies themselves
-  // form a linked list of all unresolved proxies.
-  UnresolvedList unresolved_list_;
-  // Declarations.
-  base::ThreadedList<Declaration> decls_;
-
-  // Serialized scope info support.
-  Handle<ScopeInfo> scope_info_;
-// Debugging support.
-#ifdef DEBUG
-  const AstRawString* scope_name_;
-
-  // True if it doesn't need scope resolution (e.g., if the scope was
-  // constructed based on a serialized scope info or a catch context).
-  bool already_resolved_;
-  // True if this scope may contain objects from a temp zone that needs to be
-  // fixed up.
-  bool needs_migration_;
-#endif
-
-  // Source positions.
-  int start_position_;
-  int end_position_;
-
-  // Computed via AllocateVariables.
-  int num_stack_slots_;
-  int num_heap_slots_;
-
-  // The scope type.
-  const ScopeType scope_type_;
-
-  // Scope-specific information computed during parsing.
-  //
-  // The language mode of this scope.
-  STATIC_ASSERT(LanguageModeSize == 2);
-  bool is_strict_ : 1;
-  // This scope or a nested catch scope or with scope contain an 'eval' call. At
-  // the 'eval' call site this scope is the declaration scope.
-  bool scope_calls_eval_ : 1;
-  // This scope's declarations might not be executed in order (e.g., switch).
-  bool scope_nonlinear_ : 1;
-  bool is_hidden_ : 1;
-  // Temporary workaround that allows masking of 'this' in debug-evalute scopes.
-  bool is_debug_evaluate_scope_ : 1;
-
-  // True if one of the inner scopes or the scope itself calls eval.
-  bool inner_scope_calls_eval_ : 1;
-  bool force_context_allocation_ : 1;
-  bool force_context_allocation_for_parameters_ : 1;
-
-  // True if it holds 'var' declarations.
-  bool is_declaration_scope_ : 1;
-
-  bool must_use_preparsed_scope_data_ : 1;
-
   // Create a non-local variable with a given name.
   // These variables are looked up dynamically at runtime.
   Variable* NonLocal(const AstRawString* name, VariableMode mode);
@@ -732,6 +659,77 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
 
   friend class DeclarationScope;
   friend class ScopeTestHelper;
+
+  Zone* zone_;
+
+  // Scope tree.
+  Scope* outer_scope_;  // the immediately enclosing outer scope, or nullptr
+  Scope* inner_scope_;  // an inner scope of this scope
+  Scope* sibling_;  // a sibling inner scope of the outer scope of this scope.
+
+  // The variables declared in this scope:
+  //
+  // All user-declared variables (incl. parameters).  For script scopes
+  // variables may be implicitly 'declared' by being used (possibly in
+  // an inner scope) with no intervening with statements or eval calls.
+  VariableMap variables_;
+  // In case of non-scopeinfo-backed scopes, this contains the variables of the
+  // map above in order of addition.
+  base::ThreadedList<Variable> locals_;
+  // Unresolved variables referred to from this scope. The proxies themselves
+  // form a linked list of all unresolved proxies.
+  UnresolvedList unresolved_list_;
+  // Declarations.
+  base::ThreadedList<Declaration> decls_;
+
+  // Serialized scope info support.
+  Handle<ScopeInfo> scope_info_;
+// Debugging support.
+#ifdef DEBUG
+  const AstRawString* scope_name_;
+
+  // True if it doesn't need scope resolution (e.g., if the scope was
+  // constructed based on a serialized scope info or a catch context).
+  bool already_resolved_;
+  // True if this scope may contain objects from a temp zone that needs to be
+  // fixed up.
+  bool needs_migration_;
+#endif
+
+  // Source positions.
+  int start_position_;
+  int end_position_;
+
+  // Computed via AllocateVariables.
+  int num_stack_slots_;
+  int num_heap_slots_;
+
+  // The scope type.
+  const ScopeType scope_type_;
+
+  // Scope-specific information computed during parsing.
+  //
+  // The language mode of this scope.
+  STATIC_ASSERT(LanguageModeSize == 2);
+  bool is_strict_ : 1;
+  // This scope or a nested catch scope or with scope contain an 'eval' call. At
+  // the 'eval' call site this scope is the declaration scope.
+  bool scope_calls_eval_ : 1;
+  // This scope's declarations might not be executed in order (e.g., switch).
+  bool scope_nonlinear_ : 1;
+  bool is_hidden_ : 1;
+  // Temporary workaround that allows masking of 'this' in debug-evalute scopes.
+  bool is_debug_evaluate_scope_ : 1;
+
+  // True if one of the inner scopes or the scope itself calls eval.
+  bool inner_scope_calls_eval_ : 1;
+  bool force_context_allocation_ : 1;
+  bool force_context_allocation_for_parameters_ : 1;
+
+  // True if it holds 'var' declarations.
+  bool is_declaration_scope_ : 1;
+
+  bool must_use_preparsed_scope_data_ : 1;
 };
 
 class V8_EXPORT_PRIVATE DeclarationScope : public Scope {

src/parsing/preparsed-scope-data-impl.h

@@ -170,7 +170,7 @@ class BaseConsumedPreParsedScopeData : public ConsumedPreParsedScopeData {
 #endif
 
  private:
-  void RestoreData(Scope* scope);
+  void RestoreDataForScope(Scope* scope);
   void RestoreDataForVariable(Variable* var);
   void RestoreDataForInnerScopes(Scope* scope);
 

src/parsing/preparsed-scope-data.cc

@@ -181,9 +181,7 @@ void PreParsedScopeDataBuilder::AddSkippableFunction(int start_position,
                                                      int num_inner_functions,
                                                      LanguageMode language_mode,
                                                      bool uses_super_property) {
-  if (bailed_out_) {
-    return;
-  }
+  if (bailed_out_) return;
 
   // Start position is used for a sanity check when consuming the data, we could
   // remove it in the future if we're very pressed for space but it's been good
@@ -208,9 +206,7 @@ void PreParsedScopeDataBuilder::SaveScopeAllocationData(
   DCHECK_EQ(byte_data_->size() % ByteData::kSkippableFunctionDataSize,
             ByteData::kPlaceholderSize);
 
-  if (bailed_out_) {
-    return;
-  }
+  if (bailed_out_) return;
 
   uint32_t scope_data_start = static_cast<uint32_t>(byte_data_->size());
 
@@ -229,7 +225,7 @@ void PreParsedScopeDataBuilder::SaveScopeAllocationData(
   byte_data_->WriteUint32(scope->end_position());
 #endif
 
-  SaveDataForScope(scope);
+  if (ScopeNeedsData(scope)) SaveDataForScope(scope);
 }
 
 bool PreParsedScopeDataBuilder::ContainsInnerFunctions() const {
@@ -238,9 +234,7 @@ bool PreParsedScopeDataBuilder::ContainsInnerFunctions() const {
 
 MaybeHandle<PreParsedScopeData> PreParsedScopeDataBuilder::Serialize(
     Isolate* isolate) {
-  if (bailed_out_) {
-    return MaybeHandle<PreParsedScopeData>();
-  }
+  if (bailed_out_) return MaybeHandle<PreParsedScopeData>();
 
   DCHECK(!ThisOrParentBailedOut());
 
@@ -271,9 +265,7 @@ MaybeHandle<PreParsedScopeData> PreParsedScopeDataBuilder::Serialize(
 }
 
 ZonePreParsedScopeData* PreParsedScopeDataBuilder::Serialize(Zone* zone) {
-  if (bailed_out_) {
-    return nullptr;
-  }
+  if (bailed_out_) return nullptr;
 
   DCHECK(!ThisOrParentBailedOut());
 
@@ -304,16 +296,12 @@ bool PreParsedScopeDataBuilder::ScopeNeedsData(Scope* scope) {
   }
   if (!scope->is_hidden()) {
     for (Variable* var : *scope->locals()) {
-      if (IsDeclaredVariableMode(var->mode())) {
-        return true;
-      }
+      if (IsDeclaredVariableMode(var->mode())) return true;
     }
   }
   for (Scope* inner = scope->inner_scope(); inner != nullptr;
        inner = inner->sibling()) {
-    if (ScopeNeedsData(inner)) {
-      return true;
-    }
+    if (ScopeNeedsData(inner)) return true;
   }
   return false;
 }
@@ -324,9 +312,7 @@ bool PreParsedScopeDataBuilder::ScopeIsSkippableFunctionScope(Scope* scope) {
   // logic ensures that the scope allocation data is consistent with the
   // skippable function data (both agree on where the lazy function boundaries
   // are).
-  if (scope->scope_type() != ScopeType::FUNCTION_SCOPE) {
-    return false;
-  }
+  if (scope->scope_type() != ScopeType::FUNCTION_SCOPE) return false;
   DeclarationScope* declaration_scope = scope->AsDeclarationScope();
   return !declaration_scope->is_arrow_scope() &&
          declaration_scope->preparsed_scope_data_builder() != nullptr;
@@ -334,10 +320,7 @@ bool PreParsedScopeDataBuilder::ScopeIsSkippableFunctionScope(Scope* scope) {
 
 void PreParsedScopeDataBuilder::SaveDataForScope(Scope* scope) {
   DCHECK_NE(scope->end_position(), kNoSourcePosition);
-
-  if (!ScopeNeedsData(scope)) {
-    return;
-  }
+  DCHECK(ScopeNeedsData(scope));
 
 #ifdef DEBUG
   byte_data_->WriteUint8(scope->scope_type());
@@ -388,7 +371,6 @@ void PreParsedScopeDataBuilder::SaveDataForInnerScopes(Scope* scope) {
   // Inner scopes are stored in the reverse order, but we'd like to write the
   // data in the logical order. There might be many inner scopes, so we don't
   // want to recurse here.
-  std::vector<Scope*> scopes;
   for (Scope* inner = scope->inner_scope(); inner != nullptr;
        inner = inner->sibling()) {
     if (ScopeIsSkippableFunctionScope(inner)) {
@@ -398,10 +380,8 @@ void PreParsedScopeDataBuilder::SaveDataForInnerScopes(Scope* scope) {
           inner->AsDeclarationScope()->preparsed_scope_data_builder());
       continue;
     }
-    scopes.push_back(inner);
-  }
-  for (auto it = scopes.rbegin(); it != scopes.rend(); ++it) {
-    SaveDataForScope(*it);
+    if (!ScopeNeedsData(inner)) continue;
+    SaveDataForScope(inner);
   }
 }
 
@@ -517,14 +497,14 @@ void BaseConsumedPreParsedScopeData<Data>::RestoreScopeAllocationData(
   DCHECK_EQ(end_position_from_data, scope->end_position());
 #endif
 
-  RestoreData(scope);
+  RestoreDataForScope(scope);
 
   // Check that we consumed all scope data.
   DCHECK_EQ(scope_data_->RemainingBytes(), 0);
 }
 
 template <typename Data>
-void BaseConsumedPreParsedScopeData<Data>::RestoreData(Scope* scope) {
+void BaseConsumedPreParsedScopeData<Data>::RestoreDataForScope(Scope* scope) {
   if (scope->is_declaration_scope() &&
       scope->AsDeclarationScope()->is_skipped_function()) {
     return;
@@ -533,9 +513,7 @@ void BaseConsumedPreParsedScopeData<Data>::RestoreData(Scope* scope) {
   // It's possible that scope is not present in the data at all (since PreParser
   // doesn't create the corresponding scope). In this case, the Scope won't
   // contain any variables for which we need the data.
-  if (!PreParsedScopeDataBuilder::ScopeNeedsData(scope)) {
-    return;
-  }
+  if (!PreParsedScopeDataBuilder::ScopeNeedsData(scope)) return;
 
   // scope_type is stored only in debug mode.
   CHECK_GE(scope_data_->RemainingBytes(), ByteData::kUint8Size);
@@ -551,9 +529,7 @@ void BaseConsumedPreParsedScopeData<Data>::RestoreData(Scope* scope) {
 
   if (scope->scope_type() == ScopeType::FUNCTION_SCOPE) {
     Variable* function = scope->AsDeclarationScope()->function_var();
-    if (function != nullptr) {
-      RestoreDataForVariable(function);
-    }
+    if (function != nullptr) RestoreDataForVariable(function);
   }
 
   for (Variable* var : *scope->locals()) {
@@ -604,13 +580,9 @@ void BaseConsumedPreParsedScopeData<Data>::RestoreDataForVariable(
 template <typename Data>
 void BaseConsumedPreParsedScopeData<Data>::RestoreDataForInnerScopes(
     Scope* scope) {
-  std::vector<Scope*> scopes;
   for (Scope* inner = scope->inner_scope(); inner != nullptr;
        inner = inner->sibling()) {
-    scopes.push_back(inner);
-  }
-  for (auto it = scopes.rbegin(); it != scopes.rend(); ++it) {
-    RestoreData(*it);
+    RestoreDataForScope(inner);
   }
 }
 
@@ -697,9 +669,7 @@ ProducedPreParsedScopeData* ZoneConsumedPreParsedScopeData::GetChildData(
     Zone* zone, int child_index) {
   CHECK_GT(data_->child_length(), child_index);
   ZonePreParsedScopeData* child_data = data_->get_child(child_index);
-  if (child_data == nullptr) {
-    return nullptr;
-  }
+  if (child_data == nullptr) return nullptr;
   return ProducedPreParsedScopeData::For(child_data, zone);
 }