Closed Bug 1418624 Opened 2 years ago Closed 8 months ago

fix Result::unwrap to be less terrible

Categories

(Core :: MFBT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox59 --- wontfix
firefox70 --- fixed

People

(Reporter: froydnj, Assigned: emilio)

References

Details

Attachments

(1 file)

Bug 1417101 introduces code like:

Result<Maybe<Foo>, nsresult> DoSomething(...);

Where Foo is rather large, about 20 words.  The above is then used in code:

Result<Maybe<Foo>, nsresult> r = DoSomething(...);

if (r.isErr()) {
  return;
}

Maybe<Foo> f = r.unwrap();

Result<V, E>::unwrap() returns V by value in all cases, even when it might make sense to return V&.  (Some of the underlying implementations of Result<V, E> *have* to return V by value, but several of them could just as well return V&.)  For code like the above, this causes a lot of needless copying of Foo.

Granted, even if unwrap() returned V&, the above code could still be written, and that would be unfortunate.  But as it stands, the user *has* to do the inefficient thing; we should be giving the user a fighting chance to write efficient code.

Perhaps Result should have something like Variant::extract, where it's more obvious that we're returning a (ideally moved) value, and then we should change unwrap() to return references where possible?
Duplicate of this bug: 1563301

Alternative idea: r-value ref-qualify unwrap(), so people have to write std::move(result).unwrap(), so our use-after-move analysis stops you from re-using result after unwrap()?

Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: nobody → emilio

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

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9bc4747798bc
Allow mozilla::Result to be moved, make unwrap{,Err}() move, and add inspect() APIs that return references. r=froydnj
Status: REOPENED → RESOLVED
Closed: 9 months ago8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Duplicate of this bug: 1562333
You need to log in before you can comment on or make changes to this bug.