Closed Bug 1255632 Opened 4 years ago Closed 3 years ago

Support lambdas in Maybe's higher-order functions

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: seth, Assigned: seth)

Details

Attachments

(1 file, 2 obsolete files)

Right now Maybe's higher-order functions (map and apply) only support function pointers and to some extent functors. Let's make them support lambdas. That means we can also drop the variadic arguments, since it's much cleaner for callers to just use lambda capture.
Here's the patch. The code is actually simpler.
Attachment #8729246 - Flags: review?(jwalden+bmo)
There are two existing callsites that use the form of Maybe::map that takes
variadic arguments. Since I'm removing that in favor of lambda capture, these
callsites need to be updated to use lambdas.

This must be folded into the previous patch before landing or else the build
will fail.
Attachment #8729248 - Flags: review?(n.nethercote)
Comment on attachment 8729246 [details] [diff] [review]
(Part 1a) - Make Maybe::map and Maybe::apply support lambdas.

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

::: mfbt/Maybe.h
@@ +353,3 @@
>      if (isSome()) {
> +      Maybe<ReturnType> val;
> +      val.emplace(aFunc(ref()));

Will this work with moving the old value from the old Maybe into the lambda?

  Maybe<MoveOnlyType> m(...);
  auto mapped = .map([](MoveOnlyType&& t) { return consume(mozilla::Move(t)); });
Comment on attachment 8729248 [details] [diff] [review]
(Part 1b) - Update existing Maybe::map callsites to use lambdas.

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

::: image/ClippedImage.cpp
@@ -411,5 @@
> -  nsIntSize innerSize(aInnerAndClipSize.first);
> -  nsIntSize clipSize(aInnerAndClipSize.second);
> -
> -  // Map the viewport to the inner image. (Note that we don't take the aSize
> -  // parameter of Draw into account, just the clipping region.)

This comment doesn't seem to make much sense even in the old code -- I can't tell what |aSize| and |Draw| are supposed to refer to. Please fix this appropriately if you know how to.
Attachment #8729248 - Flags: review?(n.nethercote) → review+
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #3)
> Will this work with moving the old value from the old Maybe into the lambda?
> 
>   Maybe<MoveOnlyType> m(...);
>   auto mapped = .map([](MoveOnlyType&& t) { return
> consume(mozilla::Move(t)); });

No, but that is an impressively insane thing to do. =)
(In reply to Nicholas Nethercote [:njn] from comment #4)
> This comment doesn't seem to make much sense even in the old code -- I can't
> tell what |aSize| and |Draw| are supposed to refer to. Please fix this
> appropriately if you know how to.

Yeah, I agree, this comment is impressively hard to understand. =) I'll clean it up.
(Lots of impressive stuff happening in this bug!)
(In reply to Seth Fowler [:seth] [:s2h] from comment #5)
> (In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #3)
> > Will this work with moving the old value from the old Maybe into the lambda?

Actually, this will work in the sense that it won't crash, but map() won't call the lambda since the old Maybe becomes a Nothing, so it won't do what you probably wanted.
Comment on attachment 8729246 [details] [diff] [review]
(Part 1a) - Make Maybe::map and Maybe::apply support lambdas.

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

::: mfbt/Maybe.h
@@ +356,1 @@
>        return val;

Maybe this is out for order of declaration, but you could make this

  return Some(aFunc(ref()));

or at least you could if this function were declared inline and defined out-of-line.  I think.  Seems a little nicer than declaring and emplacing, IMO.

::: mfbt/tests/TestMaybe.cpp
@@ +656,5 @@
>    MOZ_RELEASE_ASSERT(tagMultiplier.mBy.GetStatus() == eWasConstructed);
>    MOZ_RELEASE_ASSERT(mayValue.map(tagMultiplier) == Some(4));
>    MOZ_RELEASE_ASSERT(tagMultiplier.mBy.GetStatus() == eWasConstructed);
> +
> +  // Check that map works with lambda expressions.

Might be worth adding some testing of different forms of lambda.  = to check that copying works correctly (and that modifying the lambda's copy doesn't do anything), & to check that references work correctly (and htat modifying the reference in the lambda is observable in the caller).
Attachment #8729246 - Flags: review?(jwalden+bmo) → review+
I was going through my repo to find branches that could be deleted and I noticed I never landed this. Belated thanks for the reviews!

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> Comment on attachment 8729246 [details] [diff] [review]
> (Part 1a) - Make Maybe::map and Maybe::apply support lambdas.
> 
> Review of attachment 8729246 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mfbt/Maybe.h
> @@ +356,1 @@
> >        return val;
> 
> Maybe this is out for order of declaration, but you could make this
> 
>   return Some(aFunc(ref()));
> 
> or at least you could if this function were declared inline and defined
> out-of-line.  I think.  Seems a little nicer than declaring and emplacing,
> IMO.

The problem is that Some() will change the type if aFunc returns a cv-qualified or reference type. It's pretty rare that that matters, so Some() is generally fine in expressions in user code where we know the types involved, but for this use-case I think we want to avoid that.

> ::: mfbt/tests/TestMaybe.cpp
> @@ +656,5 @@
> >    MOZ_RELEASE_ASSERT(tagMultiplier.mBy.GetStatus() == eWasConstructed);
> >    MOZ_RELEASE_ASSERT(mayValue.map(tagMultiplier) == Some(4));
> >    MOZ_RELEASE_ASSERT(tagMultiplier.mBy.GetStatus() == eWasConstructed);
> > +
> > +  // Check that map works with lambda expressions.
> 
> Might be worth adding some testing of different forms of lambda.  = to check
> that copying works correctly (and that modifying the lambda's copy doesn't
> do anything), & to check that references work correctly (and htat modifying
> the reference in the lambda is observable in the caller).

Good idea; I'll add the tests.
This updated patch addresses all review comments and is folded together for
landing.
Attachment #8729246 - Attachment is obsolete: true
Attachment #8729248 - Attachment is obsolete: true
Pushed by mfowler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b03f23fdde5
Make Maybe::map and Maybe::apply support lambdas. r=waldo,njn
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/185065fdef03
Backed out changeset 7b03f23fdde5 for bustage on a CLOSED TREE
OK, the problem seems to be that clang and gcc have different behaviors when you reference |this| in decltype() in the type signature of a method. I modified the code to use declval(), which should fix this. Try job here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7b08b351b3a
Flags: needinfo?(seth)
Pushed by mfowler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb6ba3a1204
Make Maybe::map and Maybe::apply support lambdas. r=waldo,njn
https://hg.mozilla.org/mozilla-central/rev/ebb6ba3a1204
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.