Skip to content

Commit c3ee3ac

Browse files
emilioemilio
committed
Bug 1418624 - Allow mozilla::Result to be moved, make unwrap{,Err}() move, and add inspect() APIs that return references. r=froydnj
Also adjust some of the callers that were either calling unwrap() repeatedly on the same result, or were doing silly copies, to use inspect(). We could try to use stuff like: https://clang.llvm.org/docs/AttributeReference.html#consumed-annotation-checking Differential Revision: https://phabricator.services.mozilla.com/D41425 --HG-- extra : moz-landing-system : lando
1 parent 81a41b2 commit c3ee3ac

File tree

10 files changed

+141
-63
lines changed

10 files changed

+141
-63
lines changed

‎browser/app/winlauncher/ErrorHandler.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ inline void HandleLauncherError(const LauncherResult<T>& aResult) {
2828
return;
2929
}
3030

31-
HandleLauncherError(aResult.unwrapErr());
31+
HandleLauncherError(aResult.inspectErr());
3232
}
3333

3434
// This function is simply a convenience overload that unwraps the provided

‎browser/app/winlauncher/test/TestSameBinary.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ inline void PrintLauncherError(const mozilla::LauncherResult<T>& aResult,
5050
const char* aMsg = nullptr) {
5151
const char* const kSep = aMsg ? ": " : "";
5252
const char* msg = aMsg ? aMsg : "";
53-
mozilla::LauncherError err = aResult.unwrapErr();
53+
const mozilla::LauncherError& err = aResult.inspectErr();
5454
printf("%s%s%s%S (%s:%d)\n", kMsgStart, msg, kSep,
5555
err.mError.AsString().get(), err.mFile, err.mLine);
5656
}
@@ -62,13 +62,13 @@ static int ChildMain(DWORD aExpectedParentPid) {
6262
return 1;
6363
}
6464

65-
if (parentPid.unwrap() != aExpectedParentPid) {
65+
if (parentPid.inspect() != aExpectedParentPid) {
6666
PrintErrorMsg("Unexpected mismatch of parent PIDs");
6767
return 1;
6868
}
6969

7070
const DWORD kAccess = PROCESS_QUERY_LIMITED_INFORMATION | PROCESS_TERMINATE;
71-
nsAutoHandle parentProcess(::OpenProcess(kAccess, FALSE, parentPid.unwrap()));
71+
nsAutoHandle parentProcess(::OpenProcess(kAccess, FALSE, parentPid.inspect()));
7272
if (!parentProcess) {
7373
PrintWinError("Unexpectedly failed to call OpenProcess on parent");
7474
return 1;
@@ -104,7 +104,7 @@ static int ChildMain(DWORD aExpectedParentPid) {
104104
const uint32_t kMaxAttempts = 100;
105105
uint32_t curAttempt = 0;
106106
while (HANDLE p = ::OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, FALSE,
107-
parentPid.unwrap())) {
107+
parentPid.inspect())) {
108108
::CloseHandle(p);
109109
::Sleep(100);
110110
++curAttempt;

‎dom/base/nsContentSink.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -727,7 +727,7 @@ nsresult nsContentSink::ProcessStyleLinkFromHeader(
727727
return loadResultOrErr.unwrapErr();
728728
}
729729

730-
if (loadResultOrErr.unwrap().ShouldBlock() && !mRunsToCompletion) {
730+
if (loadResultOrErr.inspect().ShouldBlock() && !mRunsToCompletion) {
731731
++mPendingSheetCount;
732732
mScriptLoader->AddParserBlockingScriptExecutionBlocker();
733733
}

‎dom/media/MediaRecorder.cpp

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -431,13 +431,13 @@ class MediaRecorder::Session : public PrincipalChangeObserver<MediaStreamTrack>,
431431
}
432432

433433
if (mRunningState.isOk() &&
434-
mRunningState.unwrap() == RunningState::Idling) {
434+
mRunningState.inspect() == RunningState::Idling) {
435435
LOG(LogLevel::Debug, ("Session.Stop Explicit end task %p", this));
436436
// End the Session directly if there is no encoder.
437437
DoSessionEndTask(NS_OK);
438438
} else if (mRunningState.isOk() &&
439-
(mRunningState.unwrap() == RunningState::Starting ||
440-
mRunningState.unwrap() == RunningState::Running)) {
439+
(mRunningState.inspect() == RunningState::Starting ||
440+
mRunningState.inspect() == RunningState::Running)) {
441441
mRunningState = RunningState::Stopping;
442442
}
443443
}
@@ -632,7 +632,7 @@ class MediaRecorder::Session : public PrincipalChangeObserver<MediaStreamTrack>,
632632
}
633633

634634
if (!mRunningState.isOk() ||
635-
mRunningState.unwrap() != RunningState::Idling) {
635+
mRunningState.inspect() != RunningState::Idling) {
636636
return;
637637
}
638638

@@ -749,7 +749,7 @@ class MediaRecorder::Session : public PrincipalChangeObserver<MediaStreamTrack>,
749749
MOZ_ASSERT(NS_IsMainThread());
750750

751751
if (!mRunningState.isOk() ||
752-
mRunningState.unwrap() != RunningState::Idling) {
752+
mRunningState.inspect() != RunningState::Idling) {
753753
MOZ_ASSERT_UNREACHABLE("Double-init");
754754
return;
755755
}
@@ -894,15 +894,15 @@ class MediaRecorder::Session : public PrincipalChangeObserver<MediaStreamTrack>,
894894
}
895895

896896
if (mRunningState.isOk() &&
897-
mRunningState.unwrap() == RunningState::Stopped) {
897+
mRunningState.inspect() == RunningState::Stopped) {
898898
// We have already ended gracefully.
899899
return;
900900
}
901901

902902
bool needsStartEvent = false;
903903
if (mRunningState.isOk() &&
904-
(mRunningState.unwrap() == RunningState::Idling ||
905-
mRunningState.unwrap() == RunningState::Starting)) {
904+
(mRunningState.inspect() == RunningState::Idling ||
905+
mRunningState.inspect() == RunningState::Starting)) {
906906
needsStartEvent = true;
907907
}
908908

@@ -985,7 +985,7 @@ class MediaRecorder::Session : public PrincipalChangeObserver<MediaStreamTrack>,
985985
}
986986
mMimeType = mime;
987987
mRecorder->SetMimeType(mime);
988-
auto state = mRunningState.unwrap();
988+
RunningState state = mRunningState.inspect();
989989
if (state == RunningState::Starting || state == RunningState::Stopping) {
990990
if (state == RunningState::Starting) {
991991
// We set it to Running in the runnable since we can only assign

‎mfbt/Result.h

Lines changed: 75 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -47,16 +47,27 @@ class ResultImplementation<V, E, PackingStrategy::Variant> {
4747
mozilla::Variant<V, E> mStorage;
4848

4949
public:
50+
ResultImplementation(ResultImplementation&&) = default;
51+
ResultImplementation(const ResultImplementation&) = default;
52+
ResultImplementation& operator=(const ResultImplementation&) = default;
53+
ResultImplementation& operator=(ResultImplementation&&) = default;
54+
55+
explicit ResultImplementation(V&& aValue)
56+
: mStorage(std::forward<V>(aValue)) {}
5057
explicit ResultImplementation(const V& aValue) : mStorage(aValue) {}
51-
explicit ResultImplementation(E aErrorValue) : mStorage(aErrorValue) {}
58+
explicit ResultImplementation(E aErrorValue)
59+
: mStorage(std::forward<E>(aErrorValue)) {}
5260

5361
bool isOk() const { return mStorage.template is<V>(); }
5462

5563
// The callers of these functions will assert isOk() has the proper value, so
5664
// these functions (in all ResultImplementation specializations) don't need
5765
// to do so.
58-
V unwrap() const { return mStorage.template as<V>(); }
59-
E unwrapErr() const { return mStorage.template as<E>(); }
66+
V unwrap() { return std::move(mStorage.template as<V>()); }
67+
const V& inspect() const { return mStorage.template as<V>(); }
68+
69+
E unwrapErr() { return std::move(mStorage.template as<E>()); }
70+
const E& inspectErr() const { return mStorage.template as<E>(); }
6071
};
6172

6273
/**
@@ -68,12 +79,18 @@ class ResultImplementation<V, E&, PackingStrategy::Variant> {
6879
mozilla::Variant<V, E*> mStorage;
6980

7081
public:
82+
explicit ResultImplementation(V&& aValue)
83+
: mStorage(std::forward<V>(aValue)) {}
7184
explicit ResultImplementation(const V& aValue) : mStorage(aValue) {}
7285
explicit ResultImplementation(E& aErrorValue) : mStorage(&aErrorValue) {}
7386

7487
bool isOk() const { return mStorage.template is<V>(); }
75-
V unwrap() const { return mStorage.template as<V>(); }
76-
E& unwrapErr() const { return *mStorage.template as<E*>(); }
88+
89+
const V& inspect() const { return mStorage.template as<V>(); }
90+
V unwrap() { return std::move(mStorage.template as<V>()); }
91+
92+
E& unwrapErr() { return *mStorage.template as<E*>(); }
93+
const E& inspectErr() const { return *mStorage.template as<E*>(); }
7794
};
7895

7996
/**
@@ -90,8 +107,11 @@ class ResultImplementation<V, E&, PackingStrategy::NullIsOk> {
90107

91108
bool isOk() const { return mErrorValue == nullptr; }
92109

93-
V unwrap() const { return V(); }
94-
E& unwrapErr() const { return *mErrorValue; }
110+
const V& inspect() const = delete;
111+
V unwrap() { return V(); }
112+
113+
const E& inspectErr() const { return *mErrorValue; }
114+
E& unwrapErr() { return *mErrorValue; }
95115
};
96116

97117
/**
@@ -113,8 +133,11 @@ class ResultImplementation<V, E, PackingStrategy::NullIsOk> {
113133

114134
bool isOk() const { return mErrorValue == NullValue; }
115135

116-
V unwrap() const { return V(); }
117-
E unwrapErr() const { return mErrorValue; }
136+
const V& inspect() const = delete;
137+
V unwrap() { return V(); }
138+
139+
const E& inspectErr() const { return mErrorValue; }
140+
E unwrapErr() { return std::move(mErrorValue); }
118141
};
119142

120143
/**
@@ -139,8 +162,11 @@ class ResultImplementation<V*, E&, PackingStrategy::LowBitTagIsError> {
139162

140163
bool isOk() const { return (mBits & 1) == 0; }
141164

142-
V* unwrap() const { return reinterpret_cast<V*>(mBits); }
143-
E& unwrapErr() const { return *reinterpret_cast<E*>(mBits ^ 1); }
165+
V* inspect() const { return reinterpret_cast<V*>(mBits); }
166+
V* unwrap() { return inspect(); }
167+
168+
E& inspectErr() const { return *reinterpret_cast<E*>(mBits ^ 1); }
169+
E& unwrapErr() { return inspectErr(); }
144170
};
145171

146172
// Return true if any of the struct can fit in a word.
@@ -174,18 +200,21 @@ class ResultImplementation<V, E, PackingStrategy::PackedVariant> {
174200

175201
public:
176202
explicit ResultImplementation(V aValue) {
177-
data.v = aValue;
203+
data.v = std::move(aValue);
178204
data.ok = true;
179205
}
180206
explicit ResultImplementation(E aErrorValue) {
181-
data.e = aErrorValue;
207+
data.e = std::move(aErrorValue);
182208
data.ok = false;
183209
}
184210

185211
bool isOk() const { return data.ok; }
186212

187-
V unwrap() const { return data.v; }
188-
E unwrapErr() const { return data.e; }
213+
const V& inspect() const { return data.v; }
214+
V unwrap() { return std::move(data.v); }
215+
216+
const E& inspectErr() const { return data.e; }
217+
E unwrapErr() { return std::move(data.e); }
189218
};
190219

191220
// To use nullptr as a special value, we need the counter part to exclude zero
@@ -297,15 +326,18 @@ class MOZ_MUST_USE_TYPE Result final {
297326
Impl mImpl;
298327

299328
public:
300-
/**
301-
* Create a success result.
302-
*/
329+
/** Create a success result. */
330+
MOZ_IMPLICIT Result(V&& aValue) : mImpl(std::forward<V>(aValue)) {
331+
MOZ_ASSERT(isOk());
332+
}
333+
334+
/** Create a success result. */
303335
MOZ_IMPLICIT Result(const V& aValue) : mImpl(aValue) { MOZ_ASSERT(isOk()); }
304336

305-
/**
306-
* Create an error result.
307-
*/
308-
explicit Result(E aErrorValue) : mImpl(aErrorValue) { MOZ_ASSERT(isErr()); }
337+
/** Create an error result. */
338+
explicit Result(E aErrorValue) : mImpl(std::forward<E>(aErrorValue)) {
339+
MOZ_ASSERT(isErr());
340+
}
309341

310342
/**
311343
* Implementation detail of MOZ_TRY().
@@ -320,32 +352,44 @@ class MOZ_MUST_USE_TYPE Result final {
320352
}
321353

322354
Result(const Result&) = default;
355+
Result(Result&&) = default;
323356
Result& operator=(const Result&) = default;
357+
Result& operator=(Result&&) = default;
324358

325359
/** True if this Result is a success result. */
326360
bool isOk() const { return mImpl.isOk(); }
327361

328362
/** True if this Result is an error result. */
329363
bool isErr() const { return !mImpl.isOk(); }
330364

331-
/** Get the success value from this Result, which must be a success result. */
332-
V unwrap() const {
365+
/** Take the success value from this Result, which must be a success result.
366+
*/
367+
V unwrap() {
333368
MOZ_ASSERT(isOk());
334369
return mImpl.unwrap();
335370
}
336371

337372
/**
338-
* Get the success value from this Result, which must be a success result.
339-
* If it is an error result, then return the aValue.
373+
* Take the success value from this Result, which must be a success result.
374+
* If it is an error result, then return the aValue.
340375
*/
341-
V unwrapOr(V aValue) const { return isOk() ? mImpl.unwrap() : aValue; }
376+
V unwrapOr(V aValue) { return isOk() ? mImpl.unwrap() : std::move(aValue); }
342377

343-
/** Get the error value from this Result, which must be an error result. */
344-
E unwrapErr() const {
378+
/** Take the error value from this Result, which must be an error result. */
379+
E unwrapErr() {
345380
MOZ_ASSERT(isErr());
346381
return mImpl.unwrapErr();
347382
}
348383

384+
/** See the success value from this Result, which must be a success result. */
385+
const V& inspect() const { return mImpl.inspect(); }
386+
387+
/** See the error value from this Result, which must be an error result. */
388+
const E& inspectErr() const {
389+
MOZ_ASSERT(isErr());
390+
return mImpl.inspectErr();
391+
}
392+
349393
/**
350394
* Map a function V -> W over this result's success variant. If this result is
351395
* an error, do not invoke the function and return a copy of the error.
@@ -372,7 +416,7 @@ class MOZ_MUST_USE_TYPE Result final {
372416
* MOZ_ASSERT(res2.unwrapErr() == 5);
373417
*/
374418
template <typename F>
375-
auto map(F f) const -> Result<decltype(f(*((V*)nullptr))), E> {
419+
auto map(F f) -> Result<decltype(f(*((V*)nullptr))), E> {
376420
using RetResult = Result<decltype(f(*((V*)nullptr))), E>;
377421
return isOk() ? RetResult(f(unwrap())) : RetResult(unwrapErr());
378422
}
@@ -407,7 +451,7 @@ class MOZ_MUST_USE_TYPE Result final {
407451
*/
408452
template <typename F, typename = typename EnableIf<detail::IsResult<decltype(
409453
(*((F*)nullptr))(*((V*)nullptr)))>::value>::Type>
410-
auto andThen(F f) const -> decltype(f(*((V*)nullptr))) {
454+
auto andThen(F f) -> decltype(f(*((V*)nullptr))) {
411455
return isOk() ? f(unwrap()) : GenericErrorResult<E>(unwrapErr());
412456
}
413457
};

‎mfbt/tests/TestResult.cpp

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,13 @@
66

77
#include <string.h>
88
#include "mozilla/Result.h"
9+
#include "mozilla/UniquePtr.h"
910

1011
using mozilla::Err;
1112
using mozilla::GenericErrorResult;
1213
using mozilla::Ok;
1314
using mozilla::Result;
15+
using mozilla::UniquePtr;
1416

1517
struct Failed {
1618
int x;
@@ -224,6 +226,38 @@ static void AndThenTest() {
224226
MOZ_RELEASE_ASSERT(r3.unwrapErr() == r4.unwrapErr());
225227
}
226228

229+
using UniqueResult = Result<UniquePtr<int>, const char*>;
230+
231+
static UniqueResult UniqueTask() { return mozilla::MakeUnique<int>(3); }
232+
static UniqueResult UniqueTaskError() { return Err("bad"); }
233+
234+
static void UniquePtrTest() {
235+
{
236+
auto result = UniqueTask();
237+
MOZ_RELEASE_ASSERT(result.isOk());
238+
auto ptr = result.unwrap();
239+
MOZ_RELEASE_ASSERT(ptr);
240+
MOZ_RELEASE_ASSERT(*ptr == 3);
241+
auto moved = result.unwrap();
242+
MOZ_RELEASE_ASSERT(!moved);
243+
}
244+
245+
{
246+
auto err = UniqueTaskError();
247+
MOZ_RELEASE_ASSERT(err.isErr());
248+
auto ptr = err.unwrapOr(mozilla::MakeUnique<int>(4));
249+
MOZ_RELEASE_ASSERT(ptr);
250+
MOZ_RELEASE_ASSERT(*ptr == 4);
251+
}
252+
253+
{
254+
auto result = UniqueTaskError();
255+
result = UniqueResult(mozilla::MakeUnique<int>(6));
256+
MOZ_RELEASE_ASSERT(result.isOk());
257+
MOZ_RELEASE_ASSERT(result.inspect() && *result.inspect() == 6);
258+
}
259+
}
260+
227261
/* * */
228262

229263
int main() {
@@ -233,5 +267,6 @@ int main() {
233267
ReferenceTest();
234268
MapTest();
235269
AndThenTest();
270+
UniquePtrTest();
236271
return 0;
237272
}

0 commit comments

Comments
 (0)