Skip to content

Commit dcfccd3

Browse files
authored
Support references in ErrorOr (#4889)
### Context & Motivation The error handling utilities in `//base/error.h` are very useful for writing code with strong safety guarantees. While hardening the `Dump` debug utilities (from review in #4866), I encountered a rough edge with references and pointers. After a [brief Discord discussion in #contributing-help](https://discord.com/channels/655572317891461132/1052653651895779359/1334675462877610038), it was suggested that adding support for references to `ErrorOr` would be a good candidate to move forward. Using a reference type with the `ErrorOr` class (e.g. `ErrorOr<Node&>`) produces two errors: <ol> <li><strong><code>variant can not have a reference type as an alternative</code></strong> <ul><li>From private field: <code>std::variant&lt;Error, T&gt; val_;</code></li></ul> </li> <li><strong><code>'operator-&gt;' declared as a pointer to a reference</code></strong> <ul><li>From member function: <code>auto operator-&gt;() -&gt; T*</code></li></ul> </li> </ol> ### Changes To support reference types, both errors are resolved: 1. `std::reference_wrapper` is conditionally used for storage when `T` is a reference type 2. type trait aliases like `using ValueT = std::remove_reference_t<T>` are used to produce compatible types for methods like `auto operator->() -> ValueT*`
1 parent 6803127 commit dcfccd3

File tree

2 files changed

+31
-15
lines changed

2 files changed

+31
-15
lines changed

common/error.h

Lines changed: 25 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,9 @@
55
#ifndef CARBON_COMMON_ERROR_H_
66
#define CARBON_COMMON_ERROR_H_
77

8+
#include <functional>
89
#include <string>
10+
#include <type_traits>
911
#include <variant>
1012

1113
#include "common/check.h"
@@ -76,18 +78,29 @@ class [[nodiscard]] Error : public Printable<Error> {
7678
template <typename T>
7779
class [[nodiscard]] ErrorOr {
7880
public:
81+
using ValueT = std::remove_reference_t<T>;
82+
7983
// Constructs with an error; the error must not be Error::Success().
8084
// Implicit for easy construction on returns.
8185
// NOLINTNEXTLINE(google-explicit-constructor)
8286
ErrorOr(Error err) : val_(std::move(err)) {}
8387

88+
// Constructs with a reference.
89+
// Implicit for easy construction on returns.
90+
// NOLINTNEXTLINE(google-explicit-constructor)
91+
ErrorOr(T ref)
92+
requires std::is_reference_v<T>
93+
: val_(std::ref(ref)) {}
94+
8495
// Constructs with a value.
8596
// Implicit for easy construction on returns.
8697
// NOLINTNEXTLINE(google-explicit-constructor)
87-
ErrorOr(T val) : val_(std::move(val)) {}
98+
ErrorOr(T val)
99+
requires(!std::is_reference_v<T>)
100+
: val_(std::move(val)) {}
88101

89102
// Returns true for success.
90-
auto ok() const -> bool { return std::holds_alternative<T>(val_); }
103+
auto ok() const -> bool { return std::holds_alternative<StoredT>(val_); }
91104

92105
// Returns the contained error.
93106
// REQUIRES: `ok()` is false.
@@ -102,35 +115,32 @@ class [[nodiscard]] ErrorOr {
102115

103116
// Returns the contained value.
104117
// REQUIRES: `ok()` is true.
105-
auto operator*() -> T& {
118+
auto operator*() -> ValueT& {
106119
CARBON_CHECK(ok());
107-
return std::get<T>(val_);
120+
return std::get<StoredT>(val_);
108121
}
109122

110123
// Returns the contained value.
111124
// REQUIRES: `ok()` is true.
112-
auto operator*() const -> const T& {
125+
auto operator*() const -> const ValueT& {
113126
CARBON_CHECK(ok());
114-
return std::get<T>(val_);
127+
return std::get<StoredT>(val_);
115128
}
116129

117130
// Returns the contained value.
118131
// REQUIRES: `ok()` is true.
119-
auto operator->() -> T* {
120-
CARBON_CHECK(ok());
121-
return &std::get<T>(val_);
122-
}
132+
auto operator->() -> ValueT* { return &**this; }
123133

124134
// Returns the contained value.
125135
// REQUIRES: `ok()` is true.
126-
auto operator->() const -> const T* {
127-
CARBON_CHECK(ok());
128-
return &std::get<T>(val_);
129-
}
136+
auto operator->() const -> const ValueT* { return &**this; }
130137

131138
private:
139+
using StoredT = std::conditional_t<std::is_reference_v<T>,
140+
std::reference_wrapper<ValueT>, T>;
141+
132142
// Either an error message or a value.
133-
std::variant<Error, T> val_;
143+
std::variant<Error, StoredT> val_;
134144
};
135145

136146
// A helper class for accumulating error message and converting to

common/error_test.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,12 @@ TEST(ErrorTest, ErrorOrArrowOp) {
5050
EXPECT_EQ(err->val, 1);
5151
}
5252

53+
TEST(ErrorTest, ErrorOrReference) {
54+
Val val = {1};
55+
ErrorOr<Val&> maybe_val(val);
56+
EXPECT_EQ(maybe_val->val, 1);
57+
}
58+
5359
auto IndirectErrorOrSuccessTest() -> ErrorOr<Success> { return Success(); }
5460

5561
TEST(ErrorTest, IndirectErrorOrSuccess) {

0 commit comments

Comments
 (0)