[parser] Refactor bookmark in SkipLazyFunctionBody

src/parsing/parser.cc

Index: src/parsing/parser.cc
diff --git a/src/parsing/parser.cc b/src/parsing/parser.cc
index e9c9617da1037cd19544dc7670ab44264e64485d..0bd3df6ea1e4a4ac5af2860c6fb8df5578edebfa 100644
--- a/src/parsing/parser.cc
+++ b/src/parsing/parser.cc
@@ -4056,22 +4056,23 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
     if (formals.has_rest) arity--;
 
     // Eager or lazy parse?
-    // If is_lazily_parsed, we'll parse lazy. If we can set a bookmark, we'll
-    // pass it to SkipLazyFunctionBody, which may use it to abort lazy
-    // parsing if it suspect that wasn't a good idea. If so, or if we didn't
-    // try to lazy parse in the first place, we'll have to parse eagerly.
-    Scanner::BookmarkScope bookmark(scanner());
+    // If is_lazily_parsed, we'll parse lazily. We'll call SkipLazyFunctionBody,
+    // which may decide to abort lazy parsing if it suspect that wasn't a good

[vogelheim, 2016/08/30 16:28:42]

nitpick: suspect -> suspects

[nickie, 2016/08/31 13:23:36]

Done.

+    // idea.  If so (in which case the parser is expected to have backtracked),

[vogelheim, 2016/08/30 16:28:42]

nitpick: One space after "idea."

[nickie, 2016/08/31 13:23:36]

Done.

+    // or if we didn't try to lazy parse in the first place, we'll have to parse
+    // eagerly.
     if (is_lazily_parsed) {
-      Scanner::BookmarkScope* maybe_bookmark =
-          bookmark.Set() ? &bookmark : nullptr;
-      SkipLazyFunctionBody(&materialized_literal_count,
-                           &expected_property_count, /*CHECK_OK*/ ok,
-                           maybe_bookmark);
+      Scanner::BookmarkScope bookmark(scanner());
+      bool may_abort = bookmark.Set();
+      bool aborted =
+          SkipLazyFunctionBody(&materialized_literal_count,
+                               &expected_property_count, may_abort, CHECK_OK);
 
       materialized_literal_count += formals.materialized_literals_count +
                                     function_state.materialized_literal_count();
 
-      if (bookmark.HasBeenReset()) {
+      if (aborted) {
+        bookmark.Reset();
         // Trigger eager (re-)parsing, just below this block.
         is_lazily_parsed = false;
 
@@ -4171,10 +4172,9 @@ Expression* Parser::ParseAsyncFunctionExpression(bool* ok) {
                               language_mode(), CHECK_OK);
 }
 
-void Parser::SkipLazyFunctionBody(int* materialized_literal_count,
-                                  int* expected_property_count, bool* ok,
-                                  Scanner::BookmarkScope* bookmark) {
-  DCHECK_IMPLIES(bookmark, bookmark->HasBeenSet());
+bool Parser::SkipLazyFunctionBody(int* materialized_literal_count,
+                                  int* expected_property_count, bool may_abort,
+                                  bool* ok) {
   if (produce_cached_parse_data()) CHECK(log_);
 
   int function_block_pos = position();
@@ -4192,14 +4192,15 @@ void Parser::SkipLazyFunctionBody(int* materialized_literal_count,
       scanner()->SeekForward(entry.end_pos() - 1);
 
       scope->set_end_position(entry.end_pos());
-      Expect(Token::RBRACE, CHECK_OK_VOID);
+      Expect(Token::RBRACE, ok);
+      if (!ok) return false;

[adamk, 2016/08/30 22:13:14]

I think you mean "if (!*ok)". bool* is about my least favorite type.

Consider adding a CHECK_OK_FALSE macro?

[nickie, 2016/08/31 13:23:36]

Done.

       total_preparse_skipped_ += scope->end_position() - function_block_pos;
       *materialized_literal_count = entry.literal_count();
       *expected_property_count = entry.property_count();
       SetLanguageMode(scope, entry.language_mode());
       if (entry.uses_super_property()) scope->RecordSuperPropertyUsage();
       if (entry.calls_eval()) scope->RecordEvalCall();
-      return;
+      return false;
     }
     cached_parse_data_->Reject();
   }
@@ -4207,25 +4208,25 @@ void Parser::SkipLazyFunctionBody(int* materialized_literal_count,
   // AST. This gathers the data needed to build a lazy function.
   SingletonLogger logger;
   PreParser::PreParseResult result =
-      ParseLazyFunctionBodyWithPreParser(&logger, bookmark);
-  if (bookmark && bookmark->HasBeenReset()) {
-    return;  // Return immediately if pre-parser devided to abort parsing.
-  }
+      ParseLazyFunctionBodyWithPreParser(&logger, may_abort);
+  // Return immediately if pre-parser decided to abort parsing.
+  if (result == PreParser::kPreParseAbort) return true;
   if (result == PreParser::kPreParseStackOverflow) {
     // Propagate stack overflow.
     set_stack_overflow();
     *ok = false;
-    return;
+    return false;
   }
   if (logger.has_error()) {
     ReportMessageAt(Scanner::Location(logger.start(), logger.end()),
                     logger.message(), logger.argument_opt(),
                     logger.error_type());
     *ok = false;
-    return;
+    return false;
   }
   scope->set_end_position(logger.end());
-  Expect(Token::RBRACE, CHECK_OK_VOID);
+  Expect(Token::RBRACE, ok);
+  if (!ok) return false;

[adamk, 2016/08/30 22:13:14]

Ok, I think the same error twice is enough to convince me that you should add a
CHECK_OK_FALSE.

[nickie, 2016/08/31 13:23:36]

Done.

   total_preparse_skipped_ += scope->end_position() - function_block_pos;
   *materialized_literal_count = logger.literals();
   *expected_property_count = logger.properties();
@@ -4240,6 +4241,7 @@ void Parser::SkipLazyFunctionBody(int* materialized_literal_count,
                       *expected_property_count, language_mode(),
                       scope->uses_super_property(), scope->calls_eval());
   }
+  return false;
 }
 
 
@@ -4692,9 +4694,8 @@ ZoneList<Statement*>* Parser::ParseEagerFunctionBody(
   return result;
 }
 
-
 PreParser::PreParseResult Parser::ParseLazyFunctionBodyWithPreParser(
-    SingletonLogger* logger, Scanner::BookmarkScope* bookmark) {
+    SingletonLogger* logger, bool may_abort) {
   // This function may be called on a background thread too; record only the
   // main thread preparse times.
   if (pre_parse_timer_ != NULL) {
@@ -4721,7 +4722,7 @@ PreParser::PreParseResult Parser::ParseLazyFunctionBodyWithPreParser(
   PreParser::PreParseResult result = reusable_preparser_->PreParseLazyFunction(
       language_mode(), function_state_->kind(),
       scope()->AsDeclarationScope()->has_simple_parameters(), parsing_module_,
-      logger, bookmark, use_counts_);
+      logger, may_abort, use_counts_);
   if (pre_parse_timer_ != NULL) {
     pre_parse_timer_->Stop();
   }