From 8455b98be39d62c3021b100a547e3a1080571048 Mon Sep 17 00:00:00 2001
From: Jakob Gruber <jgruber@chromium.org>
Date: Mon, 30 Aug 2021 09:49:07 +0200
Subject: [PATCH] [parsing] Refactor MessageDetails arguments

.. to consistently support more than a single argument.

Each argument is now a tagged union that may contain an AST string, a
C string, or a JS string handle.

Change-Id: Iac8e40b717dea95a2bc2903449dab56c181702d6
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3122086
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Auto-Submit: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/main@{#76559}
---
 .../pending-compilation-error-handler.cc      | 73 +++++++++----------
 .../pending-compilation-error-handler.h       | 71 ++++++++++--------
 2 files changed, 76 insertions(+), 68 deletions(-)

diff --git a/src/parsing/pending-compilation-error-handler.cc b/src/parsing/pending-compilation-error-handler.cc
index 9b4e1ca694..4756628ca7 100644
--- a/src/parsing/pending-compilation-error-handler.cc
+++ b/src/parsing/pending-compilation-error-handler.cc
@@ -19,49 +19,52 @@ namespace internal {
 
 void PendingCompilationErrorHandler::MessageDetails::SetString(
     Handle<String> string, Isolate* isolate) {
-  DCHECK_NE(arg0_type_, kMainThreadHandle);
-  arg0_type_ = kMainThreadHandle;
-  arg0_handle_ = string;
+  DCHECK_NE(args_[0].type, kMainThreadHandle);
+  args_[0].type = kMainThreadHandle;
+  args_[0].js_string = string;
 }
 
 void PendingCompilationErrorHandler::MessageDetails::SetString(
     Handle<String> string, LocalIsolate* isolate) {
-  DCHECK_NE(arg0_type_, kMainThreadHandle);
-  arg0_type_ = kMainThreadHandle;
-  arg0_handle_ = isolate->heap()->NewPersistentHandle(string);
+  DCHECK_NE(args_[0].type, kMainThreadHandle);
+  args_[0].type = kMainThreadHandle;
+  args_[0].js_string = isolate->heap()->NewPersistentHandle(string);
 }
 
 template <typename IsolateT>
 void PendingCompilationErrorHandler::MessageDetails::Prepare(
     IsolateT* isolate) {
-  switch (arg0_type_) {
-    case kAstRawString:
-      return SetString(arg0_->string(), isolate);
+  for (int i = 0; i < kMaxArgumentCount; i++) {
+    switch (args_[i].type) {
+      case kAstRawString:
+        return SetString(args_[i].ast_string->string(), isolate);
 
-    case kNone:
-    case kConstCharString:
-      // We can delay allocation until Arg0String(isolate).
-      // TODO(leszeks): We don't actually have to transfer this string, since
-      // it's a root.
-      return;
+      case kNone:
+      case kConstCharString:
+        // We can delay allocation until ArgString(isolate).
+        return;
 
-    case kMainThreadHandle:
-      // The message details might already be prepared, so skip them if this is
-      // the case.
-      return;
+      case kMainThreadHandle:
+        // The message details might already be prepared, so skip them if this
+        // is the case.
+        return;
+    }
   }
 }
 
-Handle<String> PendingCompilationErrorHandler::MessageDetails::Arg0String(
-    Isolate* isolate) const {
-  switch (arg0_type_) {
+Handle<String> PendingCompilationErrorHandler::MessageDetails::ArgString(
+    Isolate* isolate, int index) const {
+  // `index` may be >= argc; in that case we return a default value to pass on
+  // elsewhere.
+  DCHECK_LT(index, kMaxArgumentCount);
+  switch (args_[index].type) {
     case kMainThreadHandle:
-      return arg0_handle_;
+      return args_[index].js_string;
     case kNone:
-      return isolate->factory()->undefined_string();
+      return Handle<String>::null();
     case kConstCharString:
       return isolate->factory()
-          ->NewStringFromUtf8(base::CStrVector(char_arg0_),
+          ->NewStringFromUtf8(base::CStrVector(args_[index].c_string),
                               AllocationType::kOld)
           .ToHandleChecked();
     case kAstRawString:
@@ -69,14 +72,6 @@ Handle<String> PendingCompilationErrorHandler::MessageDetails::Arg0String(
   }
 }
 
-Handle<String> PendingCompilationErrorHandler::MessageDetails::Arg1String(
-    Isolate* isolate) const {
-  if (arg1_ == nullptr) return Handle<String>::null();
-  return isolate->factory()
-      ->NewStringFromUtf8(base::CStrVector(arg1_), AllocationType::kOld)
-      .ToHandleChecked();
-}
-
 MessageLocation PendingCompilationErrorHandler::MessageDetails::GetLocation(
     Handle<Script> script) const {
   return MessageLocation(script, start_position_, end_position_);
@@ -139,8 +134,8 @@ void PendingCompilationErrorHandler::ReportWarnings(
 
   for (const MessageDetails& warning : warning_messages_) {
     MessageLocation location = warning.GetLocation(script);
-    Handle<String> argument = warning.Arg0String(isolate);
-    DCHECK(warning.Arg1String(isolate).is_null());  // Only used for errors.
+    Handle<String> argument = warning.ArgString(isolate, 0);
+    DCHECK_LT(warning.ArgCount(), 2);  // Arg1 is only used for errors.
     Handle<JSMessageObject> message =
         MessageHandler::MakeMessageObject(isolate, warning.message(), &location,
                                           argument, Handle<FixedArray>::null());
@@ -181,8 +176,8 @@ void PendingCompilationErrorHandler::ThrowPendingError(
   if (!has_pending_error_) return;
 
   MessageLocation location = error_details_.GetLocation(script);
-  Handle<String> arg0 = error_details_.Arg0String(isolate);
-  Handle<String> arg1 = error_details_.Arg1String(isolate);
+  Handle<String> arg0 = error_details_.ArgString(isolate, 0);
+  Handle<String> arg1 = error_details_.ArgString(isolate, 1);
   isolate->debug()->OnCompileError(script);
 
   Factory* factory = isolate->factory();
@@ -195,8 +190,8 @@ Handle<String> PendingCompilationErrorHandler::FormatErrorMessageForTest(
     Isolate* isolate) {
   error_details_.Prepare(isolate);
   return MessageFormatter::Format(isolate, error_details_.message(),
-                                  error_details_.Arg0String(isolate),
-                                  error_details_.Arg1String(isolate));
+                                  error_details_.ArgString(isolate, 0),
+                                  error_details_.ArgString(isolate, 1));
 }
 
 }  // namespace internal
diff --git a/src/parsing/pending-compilation-error-handler.h b/src/parsing/pending-compilation-error-handler.h
index 2e8b16fd08..9384e94df7 100644
--- a/src/parsing/pending-compilation-error-handler.h
+++ b/src/parsing/pending-compilation-error-handler.h
@@ -25,9 +25,7 @@ class Script;
 // compilation phases.
 class PendingCompilationErrorHandler {
  public:
-  PendingCompilationErrorHandler()
-      : has_pending_error_(false), stack_overflow_(false) {}
-
+  PendingCompilationErrorHandler() = default;
   PendingCompilationErrorHandler(const PendingCompilationErrorHandler&) =
       delete;
   PendingCompilationErrorHandler& operator=(
@@ -89,40 +87,45 @@ class PendingCompilationErrorHandler {
     MessageDetails()
         : start_position_(-1),
           end_position_(-1),
-          message_(MessageTemplate::kNone),
-          arg1_(nullptr),
-          arg0_type_(kNone) {}
+          message_(MessageTemplate::kNone) {}
     MessageDetails(int start_position, int end_position,
-                   MessageTemplate message, const AstRawString* arg)
+                   MessageTemplate message, const AstRawString* arg0)
         : start_position_(start_position),
           end_position_(end_position),
           message_(message),
-          arg0_(arg),
-          arg1_(nullptr),
-          arg0_type_(arg ? kAstRawString : kNone) {}
+          args_{MessageArgument{arg0}, MessageArgument{}} {}
     MessageDetails(int start_position, int end_position,
                    MessageTemplate message, const AstRawString* arg0,
                    const char* arg1)
         : start_position_(start_position),
           end_position_(end_position),
           message_(message),
-          arg0_(arg0),
-          arg1_(arg1),
-          arg0_type_(kAstRawString) {
+          args_{MessageArgument{arg0}, MessageArgument{arg1}} {
       DCHECK_NOT_NULL(arg0);
       DCHECK_NOT_NULL(arg1);
     }
     MessageDetails(int start_position, int end_position,
-                   MessageTemplate message, const char* char_arg)
+                   MessageTemplate message, const char* arg0)
         : start_position_(start_position),
           end_position_(end_position),
           message_(message),
-          char_arg0_(char_arg),
-          arg1_(nullptr),
-          arg0_type_(char_arg0_ ? kConstCharString : kNone) {}
+          args_{MessageArgument{arg0}, MessageArgument{}} {}
+
+    Handle<String> ArgString(Isolate* isolate, int index) const;
+    int ArgCount() const {
+      int argc = 0;
+      for (int i = 0; i < kMaxArgumentCount; i++) {
+        if (args_[i].type == kNone) break;
+        argc++;
+      }
+#ifdef DEBUG
+      for (int i = argc; i < kMaxArgumentCount; i++) {
+        DCHECK_EQ(args_[i].type, kNone);
+      }
+#endif  // DEBUG
+      return argc;
+    }
 
-    Handle<String> Arg0String(Isolate* isolate) const;
-    Handle<String> Arg1String(Isolate* isolate) const;
     MessageLocation GetLocation(Handle<Script> script) const;
     MessageTemplate message() const { return message_; }
 
@@ -137,22 +140,32 @@ class PendingCompilationErrorHandler {
 
     int start_position_;
     int end_position_;
+
     MessageTemplate message_;
-    union {
-      const AstRawString* arg0_;
-      const char* char_arg0_;
-      Handle<String> arg0_handle_;
+
+    struct MessageArgument final {
+      constexpr MessageArgument() : ast_string(nullptr), type(kNone) {}
+      explicit constexpr MessageArgument(const AstRawString* s)
+          : ast_string(s), type(s == nullptr ? kNone : kAstRawString) {}
+      explicit constexpr MessageArgument(const char* s)
+          : c_string(s), type(s == nullptr ? kNone : kConstCharString) {}
+
+      union {
+        const AstRawString* ast_string;
+        const char* c_string;
+        Handle<String> js_string;
+      };
+      Type type;
     };
-    // TODO(jgruber): If we ever extend functionality of arg1, refactor it to
-    // be more consistent with arg0.
-    const char* arg1_;
-    Type arg0_type_;
+
+    static constexpr int kMaxArgumentCount = 2;
+    MessageArgument args_[kMaxArgumentCount];
   };
 
   void ThrowPendingError(Isolate* isolate, Handle<Script> script) const;
 
-  bool has_pending_error_;
-  bool stack_overflow_;
+  bool has_pending_error_ = false;
+  bool stack_overflow_ = false;
   bool unidentifiable_error_ = false;
 
   MessageDetails error_details_;