Adding RTCErrorOr class to be used by ORTC APIs.

webrtc/api/rtcerror.h

+/*
+ *  Copyright 2017 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#ifndef WEBRTC_API_RTCERROR_H_
+#define WEBRTC_API_RTCERROR_H_
+
+#include <ostream>
+#include <string>
+#include <utility>  // For std::move.
+
+#include "webrtc/base/checks.h"
+#include "webrtc/base/logging.h"
+
+namespace webrtc {
+
+// Enumeration to represent distinct classes of errors that an application
+// may wish to act upon differently. These roughly map to DOMExceptions or
+// RTCError "errorDetailEnum" values in the web API, as described in the
+// comments below.
+enum class RTCErrorType {
+  // No error.
+  NONE,
+
+  // An operation is valid, but currently unsupported.
+  // Maps to OperationError DOMException.
+  UNSUPPORTED_OPERATION,
+
+  // A supplied parameter is valid, but currently unsupported.
+  // Maps to OperationError DOMException.
+  UNSUPPORTED_PARAMETER,
+
+  // General error indicating that a supplied parameter is invalid.
+  // Maps to InvalidAccessError or TypeError DOMException depending on context.
+  INVALID_PARAMETER,
+
+  // Slightly more specific than INVALID_PARAMETER; a parameter's value was
+  // outside the allowed range.
+  // Maps to RangeError DOMException.
+  INVALID_RANGE,
+
+  // Slightly more specific than INVALID_PARAMETER; an error occurred while
+  // parsing string input.
+  // Maps to SyntaxError DOMException.
+  SYNTAX_ERROR,
+
+  // The object does not support this operation in its current state.
+  // Maps to InvalidStateError DOMException.
+  INVALID_STATE,
+
+  // An attempt was made to modify the object in an invalid way.
+  // Maps to InvalidModificationError DOMException.
+  INVALID_MODIFICATION,
+
+  // An error occurred within an underlying network protocol.
+  // Maps to NetworkError DOMException.
+  NETWORK_ERROR,
+
+  // Some resource has been exhausted; file handles, hardware resources, ports,
+  // etc.
+  // Maps to OperationError DOMException.
+  RESOURCE_EXHAUSTED,
+
+  // The operation failed due to an internal error.
+  // Maps to OperationError DOMException.
+  INTERNAL_ERROR,
+};
+
+// Roughly corresponds to RTCError in the web api. Holds an error type, a
+// message, and possibly additional information specific to that error.
+//
+// Doesn't contain anything beyond a type and message now, but will in the
+// future as more errors are implemented.
+class RTCError {
+ public:
+  // Constructors.
+
+  // Creates a "no error" error.
+  RTCError() = default;
+  explicit RTCError(RTCErrorType type) : type_(type) {}
+  RTCError(RTCErrorType type, std::string message)

[tommi, 2017/02/12 12:11:22]

is |message| expected to be std::move()d?  also, use std::move in the
initializer list?

[Taylor Brandstetter, 2017/02/12 16:19:23]

Just meant to use a const ref, fixed.

+      : type_(type), message_(message) {}
+
+  // Identical to default constructed error.
+  //
+  // Preferred over the default constructor for code readability, and reducing
+  // unnecessary copies.
+  static const RTCError& OK();

[Taylor Brandstetter, 2017/02/12 02:36:37]

This is the only thing other than RTCErrorOr and CreateAndLogError that's new.

+
+  // Error type.
+  RTCErrorType type() const { return type_; }
+  void set_type(RTCErrorType type) { type_ = type; }
+
+  // Human-readable message describing the error. Shouldn't be used for
+  // anything but logging/diagnostics, since messages are not guaranteed to be
+  // stable.
+  std::string message() const { return message_; }

[tommi, 2017/02/12 12:11:21]

return const&?

[Taylor Brandstetter, 2017/02/12 16:19:23]

Done.

+  void set_message(const std::string& message) { message_ = message; }
+
+  // Convenience method for situations where you only care whether or not an
+  // error occurred.
+  bool ok() const { return type_ == RTCErrorType::NONE; }
+
+ private:
+  RTCErrorType type_ = RTCErrorType::NONE;
+  std::string message_;
+};
+
+// Outputs the error as a friendly string.
+// Update this method when adding a new error type.
+std::ostream& operator<<(std::ostream& stream, RTCErrorType error);

[tommi, 2017/02/12 12:11:22]

Streams are costly in terms of code size and cpu usage. Is this only for for
debug logging and test diagnostics?  If not, I prefer we figure out a way to do
this that does not involve iostream. We don't need all the bloat that comes with
it such as localization.

Some more information here:
https://google.github.io/styleguide/cppguide.html#Streams

[Taylor Brandstetter, 2017/02/12 16:19:23]

Yes, it's only intended for logging. I'll add a comment.

+
+// Helper method that can be used by implementations to create an error with a
+// message and log it.
+webrtc::RTCError CreateAndLogError(webrtc::RTCErrorType type,
+                                   const std::string& message,
+                                   rtc::LoggingSeverity severity);

[Taylor Brandstetter, 2017/02/12 02:36:37]

Can be used like:

return CreateAndLogError(RTCErrorType::INVALID_PARAMETER, "Foo must be between 1
and 10.");

+
+// Logs at error level.
+webrtc::RTCError CreateAndLogError(webrtc::RTCErrorType type,
+                                   const std::string& message);
+
+// RTCErrorOr<T> is the union of an RTCError object and a T object. RTCErrorOr
+// models the concept of an object that is either a usable value, or an error
+// Status explaining why such a value is not present. To this end RTCErrorOr<T>
+// does not allow its RTCErrorType value to be RTCErrorType::NONE. This is
+// enforced by a debug check in most cases.
+//
+// The primary use-case for RTCErrorOr<T> is as the return value of a function
+// which may fail. For example, CreateRtpSender will fail if the parameters
+// could not be successfully applied at the media engine level, but if
+// successful will return a unique_ptr to an RtpSender.
+//
+// Example client usage for a RTCErrorOr<std::unique_ptr<T>>:
+//
+//  RTCErrorOr<std::unique_ptr<Foo>> result = FooFactory::MakeNewFoo(arg);
+//  if (result.ok()) {
+//    std::unique_ptr<Foo> foo = result.ConsumeValue();
+//    foo->DoSomethingCool();
+//  } else {
+//    LOG(LS_ERROR) << result.error();
+//  }
+//
+// Example factory implementation returning RTCErrorOr<std::unique_ptr<T>>:
+//
+//  RTCErrorOr<std::unique_ptr<Foo>> FooFactory::MakeNewFoo(int arg) {
+//    if (arg <= 0) {
+//      return RTCError(RTCErrorType::INVALID_RANGE, "Arg must be positive");
+//    } else {
+//      return std::unique_ptr<Foo>(new Foo(arg));
+//    }
+//  }
+//
+template <typename T>
+class RTCErrorOr {
+  // Used to convert between RTCErrorOr<Foo>/RtcErrorOr<Bar>, when an implicit
+  // conversion from Foo to Bar exists.
+  template <typename U>
+  friend class RTCErrorOr;
+
+ public:
+  typedef T element_type;
+
+  // Constructs a new RTCErrorOr with RTCErrorType::NONE error. This is marked
+  // 'explicit' to try to catch cases like 'return {};', where people think
+  // RTCErrorOr<std::vector<int>> will be initialized with an empty vector,
+  // instead of a RTCErrorType::NONE error.
+  explicit RTCErrorOr() = default;
+
+  // Constructs a new RTCErrorOr with the given non-ok error. After calling
+  // this constructor, calls to value() will DCHECK-fail.
+  //
+  // NOTE: Not explicit - we want to use RTCErrorOr<T> as a return
+  // value, so it is convenient and sensible to be able to do 'return
+  // RTCError(...)' when the return type is RTCErrorOr<T>.
+  //
+  // REQUIRES: !error.ok(). This requirement is DCHECKed.
+  RTCErrorOr(const RTCError& error) : error_(error) { RTC_DCHECK(!error.ok()); }

[tommi, 2017/02/12 12:11:21]

could this be |RTCError&& error| and move ownership to RTCErrorOr?  Since
RTCError is a complex object, it would be good if the API is designed to avoid
creating copies whenever we use RTCErrorOr as the documentation suggests.

[Taylor Brandstetter, 2017/02/12 16:19:23]

Done.

+
+  // Constructs a new RTCErrorOr with the given value. After calling this
+  // constructor, calls to value() will succeed, and calls to error() will
+  // return a default-constructed RTCError.
+  //
+  // NOTE: Not explicit - we want to use RTCErrorOr<T> as a return type
+  // so it is convenient and sensible to be able to do 'return T()'
+  // when the return type is RTCErrorOr<T>.
+  RTCErrorOr(T value) : value_(std::move(value)) {}
+
+  // Copy constructor.

[tommi, 2017/02/12 12:11:21]

do we need one? (comment?)
if we could instead live with being only movable, that would be nice.

[Taylor Brandstetter, 2017/02/12 16:19:23]

I can't think of where it would be useful. I'll remove it for now.

+  RTCErrorOr(const RTCErrorOr& other) = default;
+
+  // Assignment operator.
+  RTCErrorOr& operator=(const RTCErrorOr& other) = default;
+
+  // Move constructor and move-assignment operator.
+  RTCErrorOr(RTCErrorOr&& other) = default;
+  RTCErrorOr& operator=(RTCErrorOr&& other) = default;
+
+  // Conversion constructor and assignment operator; T must be copy or move
+  // constructible from U.
+  template <typename U>
+  RTCErrorOr(RTCErrorOr<U> other)
+      : error_(std::move(other.error_)), value_(std::move(other.value_)) {}
+  template <typename U>
+  RTCErrorOr& operator=(RTCErrorOr<U> other) {
+    error_ = std::move(other.error_);
+    value_ = std::move(other.value_);
+    return *this;
+  }
+
+  // Returns a reference to our error. If this contains a T, then returns
+  // default-constructed RTCError.
+  const RTCError& error() const { return error_; }
+
+  // Returns this->error().ok()
+  bool ok() const { return error_.ok(); }
+
+  // Returns a reference to our current value, or DCHECK-fails if !this->ok().
+  const T& value() const {
+    RTC_DCHECK(ok());
+    return value_;
+  }
+  T& value() {

[tommi, 2017/02/12 12:11:21]

can you add a comment that explains when this is needed?

[Taylor Brandstetter, 2017/02/12 16:19:23]

It could be useful if the implementation, say, wants to check something about
the value of an RTCErrorOr before passing it further up the stack.

RTCErrorOr<Foo> MakeFooInternal(Params p) {
  // stuff
  return Foo(p);
}

RTCErrorOr<Foo> MakeFoo(Params p) {
  auto result = MakeFooInternal(p);
  if (result.ok()) {
    LOG(LS_INFO) << "Created Foo with ID: " << result.value().id();
  }
  return result;
}

Bad example, since that logging could be done in MakeFooInternal, but that's the
idea.

[tommi, 2017/02/12 16:27:28]

Would the |const T& value()| accessor cover that case?
I'm specifically wondering about the ability to be able to change |value|
outside of the RTCErrorOr class.

[Taylor Brandstetter, 2017/02/12 16:44:12]

There are still some cases where the non-const-ref accessor is convenient.

RTCErrorOr<Foo> MakeGeneralFoo(...);

RTCErrorOr<Foo> MakeVideoFoo(...) {
  auto result = MakeGeneralFoo(...);
  if (!result.ok()) {
    return result;
  }
  result.value().DoVideoSpecificThing();
  return result;
}

RTCErrorOr<Foo> MakeAudioFoo(...) {
  // ...
}

[tommi, 2017/02/12 18:19:32]

OK, if it's necessary, I'm not against it. What I was thinking was that if we
don't actually need it now, it's easier to add it later if we need it than
remove it after usage gets added without us noticing (e.g. if a method should
have been made const but wasn't made so since things compile without it).

+    RTC_DCHECK(ok());
+    return value_;
+  }
+
+  // Moves our current value out of this object and returns it, or DCHECK-fails
+  // if !this->ok().
+  T MoveValue() {
+    RTC_DCHECK(ok());
+    return std::move(value_);
+  }
+
+ private:
+  RTCError error_;
+  T value_;

[kwiberg-webrtc, 2017/02/14 09:51:49]

Hmm. So an RTCErrorOr always contains a T. This may be inefficient, and may be
problematic if T isn't default-constructable.

[Taylor Brandstetter, 2017/02/15 02:22:23]

I plan to use this for things that are either small or unique_ptrs, so I think
this should be fine.

+};
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_API_RTCERROR_H_