Implement a Result<T> type. Output parameters are an antipattern that should be avoided in C++. They have a minor performance overhead from the inability to use RVO, but more importantly, they're extremely unergonomic to use. Unless you're specifically passing a potentially non-empty object by reference, you should generally avoid using them. https://www.dominikgrabiec.com/posts/2022/02/20/cpp_antipattern_passing_result_array_in.html Google3 has had absl::StatusOr and RETURN_IF_ERROR / ASSIGN_OR_RETURN since I first started in 2018. With the advent of C++23, we now have std::expected. These are equivalent to rust's "?" operator. They significantly improve readability and make it much harder to write incorrect code (eg. ignoring the error value). See go/totw/121 if you're a googler for more details Change-Id: If32e4c953f9cdfe4c3764eb01970d2596a6a6964 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/25140 Commit-Queue: Matt Stark <msta@google.com> Reviewed-by: Takuto Ikuta <tikuta@google.com>
diff --git a/src/gn/config_values_generator.cc b/src/gn/config_values_generator.cc index e275066..f97b54f 100644 --- a/src/gn/config_values_generator.cc +++ b/src/gn/config_values_generator.cc
@@ -7,6 +7,7 @@ #include "base/strings/string_util.h" #include "gn/build_settings.h" #include "gn/config_values.h" +#include "gn/err.h" #include "gn/filesystem_utils.h" #include "gn/frameworks_utils.h" #include "gn/scope.h" @@ -26,9 +27,8 @@ if (!value) return; // No value, empty input and succeed. - ExtractListOfStringValues(*value, &(config_values->*accessor)(), err); - if (err->has_error()) - return; + ASSIGN_OR_RETURN_VOID((config_values->*accessor)(), err, + ExtractListOfStringValues(*value)); const auto& strings = (config_values->*accessor)(); for (size_t i = 0; i < strings.size(); i++) { @@ -69,8 +69,7 @@ return; std::vector<std::string> frameworks; - if (!ExtractListOfStringValues(*value, &frameworks, err)) - return; + ASSIGN_OR_RETURN_VOID(frameworks, err, ExtractListOfStringValues(*value)); // All strings must end with ".frameworks". for (const std::string& framework : frameworks) { @@ -97,8 +96,7 @@ return; std::vector<std::string> weak_libraries; - if (!ExtractListOfStringValues(*value, &weak_libraries, err)) - return; + ASSIGN_OR_RETURN_VOID(weak_libraries, err, ExtractListOfStringValues(*value)); // All strings must end with ".dylib". for (const std::string& weak_library : weak_libraries) {
diff --git a/src/gn/err.h b/src/gn/err.h index 098ea95..626ba30 100644 --- a/src/gn/err.h +++ b/src/gn/err.h
@@ -5,8 +5,10 @@ #ifndef TOOLS_GN_ERR_H_ #define TOOLS_GN_ERR_H_ +#include <expected> #include <memory> #include <string> +#include <utility> #include <vector> #include "gn/label.h" @@ -131,4 +133,173 @@ std::unique_ptr<ErrInfo> info_; // Non-null indicates error. }; +// "return Ok()" is far more clear than "return Err()" +inline Err Ok() { + return Err(); +} + +// A wrapper around std::expected<T, Err>. +// std::expected does not allow implicit conversions because you can have +// std::expected<T, T>. Since that isn't a problem for us, we add the +// implicit conversions. +template <typename T> +class Result : public std::expected<T, Err> { + static_assert(!std::is_void_v<T>, "Use Err instead of Result<void>"); + static_assert(!std::is_same_v<T, Err>, "Err cannot be the success case"); + + public: + using std::expected<T, Err>::expected; + + // Implicit conversion from Err (always creates an error state) + Result(Err err) : std::expected<T, Err>(std::unexpect, std::move(err)) { + // Cannot create an error result that's actually an "Ok". + DCHECK(this->error().has_error()); + } + + // Implicit conversion from T (always creates success state) + Result(const T& val) : std::expected<T, Err>(val) {} + Result(T&& val) : std::expected<T, Err>(std::move(val)) {} + + // We implement transform_error here because while it is in the C++23 spec + // (which we use), the older macOS SDK used in CI doesn't support it. + template <typename F> + constexpr Result<T> transform_error(F&& f) const& { + if (this->has_value()) { + return Result<T>(**this); + } + return Result<T>(std::forward<F>(f)(this->error())); + } + + template <typename F> + constexpr Result<T> transform_error(F&& f) && { + if (this->has_value()) { + return Result<T>(std::move(**this)); + } + return Result<T>(std::forward<F>(f)(std::move(this->error()))); + } + + // Implementation of has_error for compatibility with Err type. + bool has_error() const { return !this->has_value(); } + + // Implementation of message for compatibility with Err type. + const std::string& message() const { + DCHECK(has_error()); + return this->error().message(); + } +}; + +namespace internal { + +inline Err GetError(const Err& err) { + return err; +} + +inline Err GetError(Err&& err) { + return std::move(err); +} + +template <typename T, typename E> +inline E GetError(const std::expected<T, E>& exp) { + return exp.error(); +} + +template <typename T, typename E> +inline E GetError(std::expected<T, E>&& exp) { + return std::move(exp).error(); +} + +} // namespace internal + +// We will define the following terms: +// A "new-style" function is one which returns either Result<T> or Err (which is +// treated as Result<void>). +// A "legacy" function takes an Err* pointer as a parameter and returns a value. + +#define _STATUS_CONCAT_INNER(a, b) a##b +#define _STATUS_CONCAT(a, b) _STATUS_CONCAT_INNER(a, b) + +// Usage: In a new-style function, call +// ASSIGN_OR_RETURN(auto foo, new_style_function()) +#define ASSIGN_OR_RETURN(lhs, rexpr) \ + _ASSIGN_OR_RETURN_IMPL(_STATUS_CONCAT(_expected_value, __COUNTER__), lhs, \ + rexpr) + +#define _ASSIGN_OR_RETURN_IMPL(expected_val, lhs, rexpr) \ + auto expected_val = (rexpr); \ + if (!expected_val) { \ + return std::move(expected_val).error(); \ + } \ + lhs = std::move(*expected_val) + +// Usage: In a new-style function, call +// RETURN_IF_ERROR(new_style_function()) +#define RETURN_IF_ERROR(expr) \ + _RETURN_IF_ERROR_IMPL(_STATUS_CONCAT(_status_value, __COUNTER__), expr) + +#define _RETURN_IF_ERROR_IMPL(status_val, expr) \ + do { \ + auto status_val = (expr); \ + if (status_val.has_error()) { \ + return internal::GetError(std::move(status_val)); \ + } \ + } while (0) + +// Usage: In a legacy function returning void, call +// ASSIGN_OR_RETURN_VOID(foo, err_ptr, new_style_function()) +#define ASSIGN_OR_RETURN_VOID(lhs, err_ptr, rexpr) \ + _ASSIGN_OR_RETURN_VOID_IMPL(_STATUS_CONCAT(_expected_value, __COUNTER__), \ + lhs, err_ptr, rexpr) + +#define _ASSIGN_OR_RETURN_VOID_IMPL(expected_val, lhs, err_ptr, rexpr) \ + auto expected_val = (rexpr); \ + if (!expected_val) { \ + *(err_ptr) = std::move(expected_val).error(); \ + return; \ + } \ + lhs = std::move(*expected_val) + +// Usage: In a legacy function returning void, call +// RETURN_IF_ERROR_VOID(err_ptr, new_style_function()) +#define RETURN_IF_ERROR_VOID(err_ptr, expr) \ + _RETURN_IF_ERROR_VOID_IMPL(_STATUS_CONCAT(_status_value, __COUNTER__), \ + err_ptr, expr) + +#define _RETURN_IF_ERROR_VOID_IMPL(status_val, err_ptr, expr) \ + do { \ + auto status_val = (expr); \ + if (status_val.has_error()) { \ + *(err_ptr) = internal::GetError(std::move(status_val)); \ + return; \ + } \ + } while (0) + +// Usage: In a legacy function returning a pointer or bool, call +// ASSIGN_OR_RETURN_PTR(foo, err_ptr, new_style_function()) +#define ASSIGN_OR_RETURN_PTR(lhs, err_ptr, rexpr) \ + _ASSIGN_OR_RETURN_PTR_IMPL(_STATUS_CONCAT(_expected_value, __COUNTER__), \ + lhs, err_ptr, rexpr) + +#define _ASSIGN_OR_RETURN_PTR_IMPL(expected_val, lhs, err_ptr, rexpr) \ + auto expected_val = (rexpr); \ + if (!expected_val) { \ + *(err_ptr) = std::move(expected_val).error(); \ + return {}; \ + } \ + lhs = std::move(*expected_val) + +// Usage: In a legacy function returning a pointer or bool, call +// RETURN_IF_ERROR_PTR(err_ptr, new_style_function()) +#define RETURN_IF_ERROR_PTR(err_ptr, expr) \ + _RETURN_IF_ERROR_PTR_IMPL(_STATUS_CONCAT(_status_value, __COUNTER__), \ + err_ptr, expr) + +#define _RETURN_IF_ERROR_PTR_IMPL(status_val, err_ptr, expr) \ + do { \ + auto status_val = (expr); \ + if (status_val.has_error()) { \ + *(err_ptr) = internal::GetError(std::move(status_val)); \ + return {}; \ + } \ + } while (0) + #endif // TOOLS_GN_ERR_H_
diff --git a/src/gn/value.cc b/src/gn/value.cc index e2aa11a..db184ee 100644 --- a/src/gn/value.cc +++ b/src/gn/value.cc
@@ -259,6 +259,12 @@ return false; } +Err Value::VerifyTypeIs(Type t) const { + Err err; + VerifyTypeIs(t, &err); + return err; +} + bool Value::operator==(const Value& other) const { if (type_ != other.type_) return false;
diff --git a/src/gn/value.h b/src/gn/value.h index ec4cb5a..dab3a8e 100644 --- a/src/gn/value.h +++ b/src/gn/value.h
@@ -122,6 +122,7 @@ // Verifies that the value is of the given type. If it isn't, returns // false and sets the error. bool VerifyTypeIs(Type t, Err* err) const; + Err VerifyTypeIs(Type t) const; // Compares values. Only the "value" is compared, not the origin. Scope // values check only the contents of the current scope, and do not go to
diff --git a/src/gn/value_extractors.cc b/src/gn/value_extractors.cc index 3984d22..255970d 100644 --- a/src/gn/value_extractors.cc +++ b/src/gn/value_extractors.cc
@@ -209,19 +209,16 @@ } // namespace -bool ExtractListOfStringValues(const Value& value, - std::vector<std::string>* dest, - Err* err) { - if (!value.VerifyTypeIs(Value::LIST, err)) - return false; +Result<std::vector<std::string>> ExtractListOfStringValues(const Value& value) { + RETURN_IF_ERROR(value.VerifyTypeIs(Value::LIST)); const std::vector<Value>& input_list = value.list_value(); - dest->reserve(input_list.size()); + std::vector<std::string> dest; + dest.reserve(input_list.size()); for (const auto& item : input_list) { - if (!item.VerifyTypeIs(Value::STRING, err)) - return false; - dest->push_back(item.string_value()); + RETURN_IF_ERROR(item.VerifyTypeIs(Value::STRING)); + dest.push_back(item.string_value()); } - return true; + return dest; } bool ExtractListOfRelativeFiles(const BuildSettings* build_settings,
diff --git a/src/gn/value_extractors.h b/src/gn/value_extractors.h index 9cc9e00..2d4a439 100644 --- a/src/gn/value_extractors.h +++ b/src/gn/value_extractors.h
@@ -8,6 +8,7 @@ #include <string> #include <vector> +#include "gn/err.h" #include "gn/label_ptr.h" #include "gn/lib_file.h" #include "gn/unique_vector.h" @@ -20,10 +21,9 @@ class SourceFile; class Value; -// On failure, returns false and sets the error. -bool ExtractListOfStringValues(const Value& value, - std::vector<std::string>* dest, - Err* err); +// Extracts the list of strings from the value. +// Returns an error if the value is not a list of strings. +Result<std::vector<std::string>> ExtractListOfStringValues(const Value& value); // Looks for a list of source files relative to a given current dir. bool ExtractListOfRelativeFiles(const BuildSettings* build_settings,