[wasm] Turn Result methods into named constructors

This removes the {error} and {verror} methods of {ResultBase} and
introduces a named constructor {Error} instead. This allows to
construct an error result in a single expression, and moves {Result}
closer to a container that is initialized once and is immutable
afterwards (just the {MoveErrorFrom} method is still violating this
pattern).

R=titzer@chromium.org

Bug: v8:8238
Change-Id: Iec16c8c6d66300ee82a48e8a9e941c72ae26e202
Reviewed-on: https://chromium-review.googlesource.com/c/1293370
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Ben Titzer <titzer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#56857}

src/wasm/decoder.h

@@ -217,12 +217,11 @@ class Decoder {
   // Converts the given value to a {Result}, copying the error if necessary.
   template <typename T, typename U = typename std::remove_reference<T>::type>
   Result<U> toResult(T&& val) {
-    Result<U> result(std::forward<T>(val));
     if (failed()) {
       TRACE("Result error: %s\n", error_msg_.c_str());
-      result.error(error_offset_, std::move(error_msg_));
+      return Result<U>::Error(error_offset_, std::move(error_msg_));
     }
-    return result;
+    return Result<U>(std::forward<T>(val));
   }
 
   // Resets the boundaries of this decoder.

src/wasm/module-decoder.cc

@@ -1132,7 +1132,7 @@ class ModuleDecoderImpl : public Decoder {
       // Wrap the error message from the function decoder.
       std::ostringstream wrapped;
       wrapped << "in function " << func_name << ": " << result.error_msg();
-      result.error(result.error_offset(), wrapped.str());
+      result = DecodeResult::Error(result.error_offset(), wrapped.str());
 
       // Set error code and location, if this is the first error.
       if (intermediate_result_.ok()) {
@@ -1477,9 +1477,11 @@ ModuleResult DecodeWasmModule(const WasmFeatures& enabled,
       SELECT_WASM_COUNTER(counters, origin, wasm_decode, module_time);
   TimedHistogramScope wasm_decode_module_time_scope(counter);
   size_t size = module_end - module_start;
-  if (module_start > module_end) return ModuleResult::Error("start > end");
-  if (size >= kV8MaxWasmModuleSize)
-    return ModuleResult::Error("size > maximum module size: %zu", size);
+  CHECK_LE(module_start, module_end);
+  if (size >= kV8MaxWasmModuleSize) {
+    return ModuleResult::Error(0, "size > maximum module size (%zu): %zu",
+                               kV8MaxWasmModuleSize, size);
+  }
   // TODO(bradnelson): Improve histogram handling of size_t.
   auto size_counter =
       SELECT_WASM_COUNTER(counters, origin, wasm, module_size_bytes);
@@ -1591,14 +1593,15 @@ FunctionResult DecodeWasmFunctionForTesting(
     const WasmModule* module, const byte* function_start,
     const byte* function_end, Counters* counters) {
   size_t size = function_end - function_start;
-  if (function_start > function_end)
-    return FunctionResult::Error("start > end");
+  CHECK_LE(function_start, function_end);
   auto size_histogram = SELECT_WASM_COUNTER(counters, module->origin, wasm,
                                             function_size_bytes);
   // TODO(bradnelson): Improve histogram handling of ptrdiff_t.
   size_histogram->AddSample(static_cast<int>(size));
-  if (size > kV8MaxWasmFunctionSize)
-    return FunctionResult::Error("size > maximum function size: %zu", size);
+  if (size > kV8MaxWasmFunctionSize) {
+    return FunctionResult::Error(0, "size > maximum function size (%zu): %zu",
+                                 kV8MaxWasmFunctionSize, size);
+  }
   ModuleDecoderImpl decoder(enabled, function_start, function_end, kWasmOrigin);
   decoder.SetCounters(counters);
   return decoder.DecodeSingleFunction(zone, wire_bytes, module,

src/wasm/streaming-decoder.h

@@ -224,9 +224,7 @@ class V8_EXPORT_PRIVATE StreamingDecoder {
   }
 
   std::unique_ptr<DecodingState> Error(std::string message) {
-    DecodeResult result(nullptr);
-    result.error(module_offset_ - 1, std::move(message));
-    return Error(std::move(result));
+    return Error(DecodeResult::Error(module_offset_ - 1, std::move(message)));
   }
 
   void ProcessModuleHeader() {

src/wasm/wasm-result.cc

@@ -49,18 +49,11 @@ void PrintFToString(std::string& str, size_t str_offset, const char* format,
 
 }  // namespace
 
-void ResultBase::error(uint32_t offset, std::string error_msg) {
-  // The error message must not be empty, otherwise Result::failed() will be
-  // false.
-  DCHECK(!error_msg.empty());
-  error_offset_ = offset;
-  error_msg_ = std::move(error_msg);
-}
-
-void ResultBase::verror(const char* format, va_list args) {
-  VPrintFToString(error_msg_, 0, format, args);
-  // Assign default message such that ok() and failed() work.
-  if (error_msg_.empty() == 0) error_msg_.assign("Error");
+// static
+std::string ResultBase::FormatError(const char* format, va_list args) {
+  std::string result;
+  VPrintFToString(result, 0, format, args);
+  return result;
 }
 
 void ErrorThrower::Format(ErrorType type, const char* format, va_list args) {

src/wasm/wasm-result.h

@@ -33,17 +33,6 @@ class V8_EXPORT_PRIVATE ResultBase {
       : error_offset_(other.error_offset_),
         error_msg_(std::move(other.error_msg_)) {}
 
-  void error(uint32_t offset, std::string error_msg);
-
-  void PRINTF_FORMAT(2, 3) error(const char* format, ...) {
-    va_list args;
-    va_start(args, format);
-    verror(format, args);
-    va_end(args);
-  }
-
-  void PRINTF_FORMAT(2, 0) verror(const char* format, va_list args);
-
   void MoveErrorFrom(ResultBase& that) {
     error_offset_ = that.error_offset_;
     // Use {swap()} + {clear()} instead of move assign, as {that} might still
@@ -58,6 +47,15 @@ class V8_EXPORT_PRIVATE ResultBase {
   uint32_t error_offset() const { return error_offset_; }
   const std::string& error_msg() const { return error_msg_; }
 
+ protected:
+  ResultBase(uint32_t error_offset, std::string error_msg)
+      : error_offset_(error_offset), error_msg_(std::move(error_msg)) {
+    // The error message must not be empty, otherwise {failed()} will be false.
+    DCHECK(!error_msg_.empty());
+  }
+
+  static std::string FormatError(const char* format, va_list args);
+
  private:
   uint32_t error_offset_ = 0;
   std::string error_msg_;
@@ -78,13 +76,18 @@ class Result : public ResultBase {
 
   Result& operator=(Result&& other) V8_NOEXCEPT = default;
 
-  static Result<T> PRINTF_FORMAT(1, 2) Error(const char* format, ...) {
+  static Result<T> PRINTF_FORMAT(2, 3)
+      Error(uint32_t offset, const char* format, ...) {
     va_list args;
     va_start(args, format);
-    Result<T> result;
-    result.verror(format, args);
+    Result<T> error_result{offset, FormatError(format, args)};
     va_end(args);
-    return result;
+    return error_result;
+  }
+
+  static Result<T> Error(uint32_t error_offset, std::string error_msg) {
+    // Call private constructor.
+    return Result<T>{error_offset, std::move(error_msg)};
   }
 
   // Accessor for the value. Returns const reference if {this} is l-value or
@@ -103,6 +106,9 @@ class Result : public ResultBase {
  private:
   T value_ = T{};
 
+  Result(uint32_t error_offset, std::string error_msg)
+      : ResultBase(error_offset, std::move(error_msg)) {}
+
   DISALLOW_COPY_AND_ASSIGN(Result);
 };