Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Given a Result<T, E> and a function T -> U, we should produce a Result<U, E>.
Assignee: nobody → nfitzgerald
Comment on attachment 8820406 [details] [diff] [review]
Add the `mozilla::Result::map` method

Review of attachment 8820406 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Result.h
@@ +246,5 @@
> +   *     MOZ_ASSERT(res2.isErr());
> +   *     MOZ_ASSERT(res2.unwrapErr() == 5);
> +   */
> +  template<typename F>
> +  auto map(F f) const -> Result<decltype(f(unwrap())), E> {

oh c++ how I have missed you
Comment on attachment 8820406 [details] [diff] [review]
Add the `mozilla::Result::map` method

Review of attachment 8820406 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, thanks for the addition.
Attachment #8820406 - Flags: review?(nfroyd) → review+
Oh no, looks like some of our compilers don't like that decltype usage, despite https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code saying that we can use it now.

Maybe there is another way...?
Fixed the decltype so everyone is happy. Carrying the r+.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=34828f66f47d
Attachment #8820422 - Flags: review+
Comment on attachment 8820422 [details] [diff] [review]
Add the `mozilla::Result::map` method

Review of attachment 8820422 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Result.h
@@ +250,5 @@
> +  auto map(F f) const -> Result<decltype(f(*((V*) nullptr))), E> {
> +      using RetResult = Result<decltype(f(*((V*) nullptr))), E>;
> +      return isOk()
> +          ? RetResult(f(unwrap()))
> +          : RetResult(unwrapErr());

I'd probably one-line this.

I wondered, in reading bugmail, if the decltype just needed to be futzed with a little.  As to the types here, I somewhat wonder if these are *actually* exactly correct -- I somewhat expect something along the lines of detail::SelectVariantType in Variant.h would be necessary for this.  But maybe that sort of thing will start showing up as an issue in the longer run, and not immediately for simpler uses.

::: mfbt/tests/TestResult.cpp
@@ +172,5 @@
> +
> +  // Mapping over success values.
> +  Result<int, MyError> res(5);
> +  bool invoked = false;
> +  auto res2 = res.map([&](int x) {

&invoked, please -- shu over in JS-land has led me around to concluding enumerating all the different capture semantics individually is better than catch-alls.
Fixes Waldo's nits.

I see there are still some compilation failures on try, although less than
before.
Attachment #8820460 - Flags: review+
Attachment #8820406 - Attachment is obsolete: true
Attachment #8820422 - Attachment is obsolete: true
Ah, it was that I forgot `explicit` on a constructor.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c085b9b6ca25
Attachment #8820463 - Flags: review+
Attachment #8820460 - Attachment is obsolete: true
Pushed by nfitzgerald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/261f9a8cc0fa
Add the `mozilla::Result::map` method; r=froydnj
https://hg.mozilla.org/mozilla-central/rev/261f9a8cc0fa
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.