Scope tree serialization and ScopeIterator cleanup.

src/parser.cc

 // Copyright 2011 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 453 matching lines @@
 // top scope to the incoming scope, and the destructor resets it. Additionally,
 // LexicalScope stores transient information used during parsing.
 
 
 class SaveScope BASE_EMBEDDED {
  public:
   SaveScope(Parser* parser, Scope* scope)
       : parser_(parser),
         previous_top_scope_(parser->top_scope_) {
     parser->top_scope_ = scope;
+    scope->SetSourceBegStatementPos(parser_->scanner().location().beg_pos);

[Kevin Millikin (Chromium), 2011/10/05 08:43:36]

I'm leery of setting this implicitly.  It's too easy for someone who just wants
to modify the parser to misunderstand what's going on.

If we commit this, we must have cctests that will break if the wrong positions
are recorded.

It's a bit less convenient, but better, to have explicit calls in the parser to
set the positions.

[Steven, 2011/10/06 19:09:27]

Doing it explicitly now as it also gives me more control over which positions I
actually record. Also added a cctest for that.
On 2011/10/05 08:43:36, Kevin Millikin wrote:

   }
 
   ~SaveScope() {
+    parser_->top_scope_->SetSourceEndStatementPos(
+        parser_->scanner().location().beg_pos);
     parser_->top_scope_ = previous_top_scope_;
   }
 
  private:
   // Bookkeeping
   Parser* parser_;
   // Previous values
   Scope* previous_top_scope_;
 };
 
@@ skipping 3389 matching lines @@
                                   materialized_literal_count,
                                   expected_property_count,
                                   only_simple_this_property_assignments,
                                   this_property_assignments,
                                   num_parameters,
                                   start_pos,
                                   end_pos,
                                   type,
                                   has_duplicate_parameters);
   function_literal->set_function_token_position(function_token_position);
+  scope->SetSourceBegStatementPos(start_pos);

[Kevin Millikin (Chromium), 2011/10/05 08:43:36]

It's a bit warty that some scopes have positions set explicitly and some
implicitly.

I recommend always doing it explicitly as described above.

If it should be implicit, then it should also be that way here (e.g., by having
the LexicalScope constructor and destructor set the start and end positions).

This code can also be improved.  Since the scope records start/end positions, we
don't really need redundant local variables for them.

We also might get away with not passing them as extra parameters to
FunctionLiteral (or even storing them in FunctionLiteral) since they're
redundantly on the scope.

[Steven, 2011/10/06 19:09:27]

Removed the local variables and removed them from FunctionLiteral.
On 2011/10/05 08:43:36, Kevin Millikin wrote:

+  scope->SetSourceEndStatementPos(end_pos);
 
   if (fni_ != NULL && should_infer_name) fni_->AddFunction(function_literal);
   return function_literal;
 }
 
 
 Expression* Parser::ParseV8Intrinsic(bool* ok) {
   // CallRuntime ::
   //   '%' Identifier Arguments
 
@@ skipping 1311 matching lines @@
       result = parser.ParseProgram(source,
                                    info->is_global(),
                                    info->StrictMode());
     }
   }
   info->SetFunction(result);
   return (result != NULL);
 }
 
 } }  // namespace v8::internal