913586 - Make Maybe<T> allow copying, moving, and have a more sane interface

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Seth Fowler [:seth] [:s2h]]

The design of a lot of interfaces in Gecko has been warped by the lack of something like boost::optional. We have horrible things in the code like arguments passed using pointers rather than by value just to encapsulate the fact that they're optional. There are cases where we have out-args just because of this problem (since using pointers requires heap allocation if you use it for return values).

It's possible that making Optional<T> a subclass of Maybe<T> would suffice, but I'm not sure about that; need to take another look at Maybe<T>.

Comment 1

[Luke Wagner [:luke]]

Maybe was never really designed, it just sorta accreted stuff.  It started as a little one-off utility I made called LazilyConstructed that had like 3 member functions.  Since LazilyConstructed was only for constructing, copy/assignment didn't make sense.  But now that it is named Maybe and used as a Maybe type, I hit this limitation all the time and I've almost added this functionality on 3 different occasions (until the need went away for some other reason).  Also welcome IMHO would be operator* and operator-> (which assert constructed-ness).

So, all I'm saying is: I for one would welcome a thoughtful interface makeover of Maybe, rather than introducing another type to make up for Maybe's faults.

Comment 2

[Seth Fowler [:seth] [:s2h]]

(In reply to Luke Wagner [:luke] from comment #1)
> So, all I'm saying is: I for one would welcome a thoughtful interface
> makeover of Maybe, rather than introducing another type to make up for
> Maybe's faults.

This is actually what I originally wanted to do, but I thought that this would be the most conservative route. If there's support for just fixing Maybe, though, I'm all for it!

Comment 3

[Seth Fowler [:seth] [:s2h]]

Ah, heck, let's do it.

Comment 4

[Seth Fowler [:seth] [:s2h]]

(Especially since Maybe<T>, Optional<T>, and Nullable<T> already exist.)

Regarding the interface changes: totally agreed on operator* and operator->. IMO we also need operator bool (if that doesn't already exist), operator==, and operator<. The two relational operators should be templates outside of the class so that they are only instantiated if invoked; they should have the natural behavior w.r.t. the underlying type's relational operators, if they exist. (And of course, they'd be undefined if they underlying type's relational operators are undefined - that's why they need to be separate template functions.)

Comment 5

[Luke Wagner [:luke]]

(In reply to Seth Fowler [:seth] from comment #4)

I agree on a bool-ish coercion, but "operator bool" is danger for the various mistakes it covers up.  Grep for "ConvertibleToBool" in mfbt for the hack we tend to use.

As for the relational operations, I'm not quite so sure.  The good thing about * and -> is that they leave a permanent syntactic clue that you are cracking open this Maybe (and therefore asserting it's non-emptiness).  For ==,<,> (and, if we provided those, eventually all the arith operators), this is lost.  Also, with operator*, it's only two chars extra to write *a == *b.

Comment 6

[Luke Wagner [:luke]]

Also, wow re: Optional<T> and Nullable<T> already existing.  Perhaps someone crazy like Ms2ger would be interested in unifying these...

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Luke Wagner [:luke] from comment #6)
> Also, wow re: Optional<T> and Nullable<T> already existing.  Perhaps someone
> crazy like Ms2ger would be interested in unifying these...

Not quite crazy enough to try to unify two concepts that don't have anything to do with each other :)

Comment 8

[Luke Wagner [:luke]]

(In reply to :Ms2ger from comment #7)
From the name, I thought maybe Nullable<T> wrapped a pointer to T, but indeed it stores a T value.  That makes it quite a lot like Maybe, except that it default-constructs T unconditionally.  I haven't looked into Optional.

Comment 9

[Seth Fowler [:seth] [:s2h]]

(In reply to Luke Wagner [:luke] from comment #5)
> As for the relational operations, I'm not quite so sure.  The good thing
> about * and -> is that they leave a permanent syntactic clue that you are
> cracking open this Maybe (and therefore asserting it's non-emptiness).  For
> ==,<,> (and, if we provided those, eventually all the arith operators), this
> is lost.  Also, with operator*, it's only two chars extra to write *a == *b.

I agree that supporting operator==(Maybe<T> a, T b) and so forth is bad. But operator==(Maybe<T> a, Maybe<T> b) is different; that's why we should support it. This doesn't necessarily involve cracking open the Maybe. For operator==, the meaning is:

- If both are 'None', true.
- If both are 'Some', the result of operator==(*a, *b).
- Otherwise, false.

This is quite useful in its own right and because it enables Maybe's to be stored in containers, and I think we should support it. The behavior for operator< would be analogous; basically None gets sorted before all Some's (or after; it's arbitrary).

Comment 10

[Luke Wagner [:luke]]

Ah, I see, so you're proposing to lift these operators from T to Maybe<T> by defining what happens on None.  That's interesting.  Looks like the Haskell functor whose name we've borrowed does the same thing:
  http://hackage.haskell.org/packages/archive/base/4.2.0.1/doc/html/Data-Maybe.html
So I guess we wouldn't be totally crazy to do the same thing :)

Comment 11

[Seth Fowler [:seth] [:s2h]]

Heh, you've seen through me - I'm definitely inspired by Haskell here. =)

Comment 12

[Luke Wagner [:luke]]

http://www.youtube.com/watch?v=-Xih5HyFOYg

Comment 13

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Revamp Maybe<T> to be usable in many more situations

Alright, it's time to do this. Not having a real Maybe<T> is something that just keeps biting me.

This patch implements everything discussed in this bug - copy/move support, equality and relational operators, implicit conversion to bool, and a cleanup of the API. It also adds type-inferred convenience functions for constructing Maybe<T> values (|Some()| and |None()|) and some basic utility methods familiar from the functional programming world. (|apply()|, |map()|, the |...Default()| functions). The functional support will become very useful once we can use more C++11 features, like std::function and lambdas, in Mozilla code, but even this basic support will be handy.

I've tried to name things according to STL patterns where it was appropriate (so |construct()| becomes |emplace()| and |destroy()| becomes |reset()|), and functional patterns when that seems more suitable (hence |apply()| and |map()|). The choice of Some and None is inspired by Scala and Rust; I think it's much more intuitive than Haskell's Just and Nothing.

Part 2 will be a patch that updates existing callers to use the new method names. (All of the existing behavior is still supported, so that's all that really needs to change.) I want to get an r+ on this part first, though, since any changes will inevitably affect part 2.

Comment 14

[Luke Wagner [:luke]]

Comment on attachment 8458399 [details] [diff] [review]
(Part 1) - Revamp Maybe<T> to be usable in many more situations

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

Looks delightful; can't wait to use!

::: mfbt/Maybe.h
@@ +291,5 @@
> +    }
> +
> +    /* Variant of |apply()| that takes an additional argument for the function. */
> +    template<typename F, typename A>
> +    void apply(F aFunc, A aArg) {

Probably want to use some rval-ref-powered perfect forwarding here.

Comment 15

[Seth Fowler [:seth] [:s2h]]

(In reply to Luke Wagner [:luke] from comment #14)
> Probably want to use some rval-ref-powered perfect forwarding here.

Thanks, Luke! I'll make that change in the final version of the patch.

Comment 16

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Revamp Maybe<T> to be usable in many more situations

Popped down to this patch in my queue and I figured I'd address Luke's comment.

As long as I was at it, I also added perfect forwarding support for |emplace()|.

I made one other tiny change: now you can pass a value to the two-argument version of |map()| (and similar for |apply()|) that does not have the same type as the argument type of the function you pass in, but is merely convertible to the function's argument type. I realized that not doing it that way made the template not match in situations where you'd expect it to.

Comment 17

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Revamp Maybe<T> to be usable in many more situations

Typo fix.

Comment 18

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Revamp Maybe<T> to be usable in many more situations

I ran into a couple of platform specific issues when testing this patch - mostly significantly, that X11 #define's 'None'. This version of the patch resolves that issue.

Comment 19

[Seth Fowler [:seth] [:s2h]]

Chris Peterson raised the question on IRC of whether callers would run still run into problems with the X11 headers, since my solution only sanitizes the header.

The answer is that they would, but it would be caught statically (they'd get a type error). They could work around it by #undef'ing None, if they so chose, or just by using the normal Maybe constructors. After all, Some() and None() are just convenience functions that aren't actually needed to use Maybe functionality - they just make the code cleaner.

I already have a patch that moves all of Gecko over to the new Maybe<T> API, and no callers actually run into this problem. The only reason there were a couple of compile errors is because a series of transitive include brought in Maybe.h even though nothing in the .cpp file actually used it.

Comment 20

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Revamp Maybe<T> to be usable in many more situations

Rebase. Whitespace changes only.

Comment 21

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Update Maybe users in accessible

Here come updates for callers to the new API.

Comment 22

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Update Maybe users in content

Comment 23

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Update Maybe users in dom

Comment 24

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 5) - Update Maybe users in image

Comment 25

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 6) - Update Maybe users in js

Comment 26

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 7) - Update Maybe users in layout

Comment 27

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 8) - Update Maybe users in netwerk

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8460660 [details] [diff] [review]
(Part 4) - Update Maybe users in dom

The choice of when to use isNone() and when to use the operator that converts to boolean seems slightly random; most glaringly so in the DOMString changes (e.g. DOMString::IsNull)...  I'm not quite sure how to improve this, though, short of dropping that operator completely (or getting rid of the isNone/isSome methods).

r=me

Comment 29

[Seth Fowler [:seth] [:s2h]]

(In reply to Boris Zbarsky [:bz] from comment #28)
> Comment on attachment 8460660 [details] [diff] [review]
> (Part 4) - Update Maybe users in dom
> 
> The choice of when to use isNone() and when to use the operator that
> converts to boolean seems slightly random; most glaringly so in the
> DOMString changes (e.g. DOMString::IsNull)...  I'm not quite sure how to
> improve this, though, short of dropping that operator completely (or getting
> rid of the isNone/isSome methods).
> 
> r=me

I'll take another look and try to make sure it makes sense. What I had intended, at least, was to use isNone()/isSome() only in cases where I thought readability was harmed by using the boolean conversion. The two cases I remember where I preferred the explicit methods were assertions, and functions like this:

> bool foo() {
>   return mMaybeVal.isSome();
> }

which I thought would mislead the reader if the body just read |return mMaybeVal;|.

Comment 30

[Luke Wagner [:luke]]

Comment on attachment 8460663 [details] [diff] [review]
(Part 6) - Update Maybe users in js

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

Looks good for the js/src parts; probably should have bholley review the xpconnect bits.

Comment 31

[Bobby Holley (:bholley)]

Comment on attachment 8460663 [details] [diff] [review]
(Part 6) - Update Maybe users in js

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

r=bholley on the XPConnect bits.

Comment 32

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8460659 [details] [diff] [review]
(Part 3) - Update Maybe users in content

Reassigning the content/ patch from Ben to Bobby since Ben hasn't been on IRC in a week and that makes me think he may be away. (But if I'm wrong about that, Ben, don't let me stop you from reviewing the patch! =)

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8460664 [details] [diff] [review]
(Part 7) - Update Maybe users in layout

r=dbaron, although the name emplace seems a little odd to me.

(I'm also not convinced that you need to go through and find a module owner or peer for each module for tree-wide API changes like this; I think it's ok for owners/peers of the module that provides an API to review trivial changes at the callers.)

Comment 34

[Seth Fowler [:seth] [:s2h]]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #33)
> Comment on attachment 8460664 [details] [diff] [review]
> (Part 7) - Update Maybe users in layout
> (I'm also not convinced that you need to go through and find a module owner
> or peer for each module for tree-wide API changes like this; I think it's ok
> for owners/peers of the module that provides an API to review trivial
> changes at the callers.)

Maybe so; I'm just imitating what I see others do. =)

Comment 35

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8460315 [details] [diff] [review]
(Part 1) - Revamp Maybe<T> to be usable in many more situations

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

Is it possible to add a comment that describes the differences between Maybe<T> and the forth coming c++ optional<T>? (http://open-std.org/JTC1/SC22/WG21/docs/papers/2014/n4081.html#optional)

Comment 36

[Seth Fowler [:seth] [:s2h]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #35)
> Is it possible to add a comment that describes the differences between
> Maybe<T> and the forth coming c++ optional<T>?
> (http://open-std.org/JTC1/SC22/WG21/docs/papers/2014/n4081.html#optional)

Sure. To answer the question here as well, the new Maybe<T> features are partly inspired by optional<T>, so there is a lot of overlap. The primary differences are:

(1) Maybe<T> has some additional convenience functions for readability. |isSome()| and |isNone()| give you the same information as operator bool(), and reset() does the same thing as |maybeValue = None()| (or in optional<T> terms, |optionalValue = nullopt|). It has has |Some()| and |None()| functions which infer the Maybe<T> type you want, making Maybe<T> less verbose to use than optional<T>. (You could define those functions for optional<T> too, though.)

(2) optional<T> allows comparisons between optional<T> and T. We deliberately do not allow that.

(3) optional<T> allow construction in-place without a separate call to |emplace()| by using a dummy |in_place_t| value as the first argument to its constructor. I considered something similar but I found it kind of ugly so I didn't include it. If others want it though, I'm not against adding it.

(4) optional<T> has |valueOr()|, the equivalent of Maybe<T>'s |valueDefault()| (and actually, I think valueOr is a better name; I might steal that) but it doesn't have the equivalents of Maybe<T>'s |refDefault()| or |ptrDefault()|.

(5) optional<T> totally lacks |map()| and |apply()|, so its support for programming in a functional style is much weaker than Maybe<T>. (Maybe<T> would be even stronger here if we could use lambdas and std::function in Mozilla code...)

Comment 37

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Revamp Maybe<T> to be usable in many more situations

Here's a new version with some small tweaks. In particular:

- I added the comment comparing Maybe to std::optional, as requested by Jeff.
- I renamed valDefault to valOr (and made the corresponding changes for the
  other *Default functions). I stole the name from std::optional; I like it better.
- I explicitly implemented the rest of the relational operators so people don't
  need to use std::rel_ops.

Comment 38

[Eric Rahm [:erahm]]

There's discussion in bug 1025173, comment 11 of using Maybe<T> in Nullable<T>. The constraint is:
> Only if they reorder the members and promise to keep them reordered

Does that seem reasonable? I'm not sure what order :bz was asking for, so pinging him here.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Basically, Nullable has the boolean first, then the value storage.  That allows us to do some games with casting from a const Nullable<nsAutoTArray> to a const Nullable<nsTArray>, since the binary layout is compatible.  If the boolean comes after the data storage, that doesn't work.

Comment 40

[Seth Fowler [:seth] [:s2h]]

(In reply to Boris Zbarsky [:bz] from comment #39)
> Basically, Nullable has the boolean first, then the value storage.  That
> allows us to do some games with casting from a const Nullable<nsAutoTArray>
> to a const Nullable<nsTArray>, since the binary layout is compatible.  If
> the boolean comes after the data storage, that doesn't work.

I don't have any problem with that change. It sounds like it'll make Maybe<T> useful in more places, so let's do it.

Comment 41

[Jeff Walden [:Waldo]]

Comment on attachment 8463691 [details] [diff] [review]
(Part 1) - Revamp Maybe<T> to be usable in many more situations

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

In the interests of simplifying your life, I suggest doing just a Maybe that has emplace, ref/value/ptr, and the constructors, only, as a patch.  Then, atop that, have a patch that adds all the valueOr, refOr, ptrOr, map, apply, stuff.  The former things are all that you care about, wrt landing this and not having to worry about rebases of the tree-wide changes.  The latter parts can safely be deferred til after the tree-wide changes.  Aside from smallish niggly things, and the Nil name, I don't think there was anything requiring any particular delay here in landing, for the former parts.

And actually, this'd be nice in terms of not having to review quite the entire thing all at once a second time, to be honest.  :-)

Also, this needs a C++ test, to make sure all the things work correctly.  I'm fine without one for most of the first-part changes mentioned above.  But emplace and all the second-part changes really need some exercise.  (I suggest using Maybe<UniquePtr<T>> to test stuff with a type that's only movable and not copyable.)  I'll let emplace slide in without a test as part 1, but the rest of this needs a test to land (and the test should pick up the emplace slack while it's at it).

::: mfbt/Maybe.h
@@ +17,5 @@
> +#include <new>  // for placement new
> +
> +// The X11 headers define 'None' as a macro, so undefine it inside this header.
> +#pragma push_macro("None")
> +#undef None

I'm not really a fan of this; users shouldn't have to inexplicably go out of their way to #undef None if they want this header.  And what happens when code legitimately using X11.h wants to use this?  And what happens if this header is included before X11.h is included?  Better to sidestep the mess with a different name.  Nil?

@@ +27,5 @@
> +template<typename T>
> +struct TemporarySomeValue
> +{
> +  TemporarySomeValue(const T& aValue) : value(aValue) { }
> +  const T& value;

Having Some return Maybe means we can have perfect forwarding, which I don't believe is possible with this code the way it's structured right now.  Well, actually I guess you could have an rvalue reference in here as a member, which is apparently a thing even tho it's kind of incredibly scary and all:

http://stackoverflow.com/questions/4774420/use-of-rvalue-reference-members

...and I'm just going to pretend I never saw that use case.  :-)

@@ +43,5 @@
> +  return aValue;
> +}
> +
> +template<typename T>
> +T& EvaluateFunctionOrReference(T(*aFunc)())

template<typename F>
auto
EvaluateFunctionOrReference(F&& aFunc) -> decltype(aFunc())
{
  return aFunc();
}

to handle movable functors.  I think this lets you get rid of having two different aFunc overloads here, as well.

@@ +49,5 @@
> +  return aFunc();
> +}
> +
> +template<typename T>
> +T& EvaluateFunctionOrReference(T& aRef)

I believe the (T&) and (const T&) overloads here can/should be replaced with a single

template<typename T>
T&&
EvaluateFunctionOrValue(T&& aValue)
{
  return Forward<T>(aValue);
}

so that movable T (more precisely, movable-*only* T like UniquePtr) are supported.  Generally in generic code it's a mistake to see |const T&| and |T| overloads, except for the case of implementing (or deleting) the copy constructor (which must be non-templated in order to be considered a copy constructor), is the rule as far as I've observed.

...wait.  So the idea is you can call |maybe.valueOr(...)| with either a T (or something that converts to it), or with a function that'll be called instead?  This inherent interface ambiguity about whether to treat the value as a value, or as a functor, seems wrong to me.  I don't see anything wrong with having a version that takes a function and calls it in the "or" case, but I don't think we should do this sort of argument-type-sniffing.  We should have two different functions, with different names, instead.  valueOr for the value case, valueOrResult for the other?  (And so on for map/apply/etc.)  Not sure about naming, just throwing out ideas.

@@ +60,5 @@
> +struct None { };
> +
> +template<typename T>
> +detail::TemporarySomeValue<T>
> +Some(const T& aValue)

Why is detail::TemporarySomeValue<T> the return type here?  Looks to me like this could just be Maybe<T>, if you moved this definition below the definition of Maybe.

And with moving support, then, why not

template<typename T>
Maybe<T>
Some(T&& aValue)
{
  return Maybe<T>::some(Forward<T>(aValue));
}

overall?

@@ +80,5 @@
> + * >     aFoo->takeAction();      // and then use |aFoo->| to access it.
> + * > }                            // |*aFoo| also works!
> + * >
> + * > doSomething(None());         // Passes a Maybe<Foo> containing no value.
> + * > doSomething(Some(Foo(100))); // Passes a Maybe<Foo> containing |Foo(100)|.

The > here hurt readability more than they help; please remove them and rely purely on the two-space indentation.

@@ +84,5 @@
> + * > doSomething(Some(Foo(100))); // Passes a Maybe<Foo> containing |Foo(100)|.
> + *
> + * Note that |None()| and |Some()| are only appropriate for use with copyable
> + * types. For types that can't be copied or where copying is expensive, use
> + * |emplace()|.

With the perfect forwarding bits I suggest a bunch, I don't think there's any reason this has to be true.  (Well, mostly.  If someone's doing Maybe<int[5000]> I guess it's still true, but they're kind of asking for it in that case.)

@@ +106,5 @@
> + * languages (e.g. Haskell's Maybe and Rust's Option). In the C++ world it's
> + * very similar to std::optional, which was proposed for C++14 and originated in
> + * Boost. The most important differences between Maybe and std::optional are:
> + *
> + *   - std::optional<T> may be compared with T. We deliberately forbid that.

sgtm!

@@ +126,5 @@
>  template<class T>
>  class Maybe
>  {
> +  typedef void (Maybe::* ConvertibleToBool)();
> +  void nonNull() {}

I'd add some weird argument types here just for greater uniqueness of the pointer type.  There might well be another function with this type in this class for all my memory holds at this point from the reading I've done, where I certainly wouldn't say the same about it if s/void/float*/ and s/\(\)/\(double, bool***, int*\)/ or so.

@@ +136,1 @@
>    {

I wouldn't bother with this myself, would just use Maybe() directly, tho I don't hugely care if it's here.

@@ +138,4 @@
>    }
>  
>    template<class T1>
> +  static Maybe some(const T1& t1)

T&& t1 and Forward<T>(t1) into emplace, so cheaply-movable things get that benefit.

@@ +145,5 @@
> +    return value;
> +  }
> +
> +  AlignedStorage2<T> mStorage;
> +  bool               mIsSome;

Don't bother with the vertical alignment, I think.

@@ +154,5 @@
> +
> +  /* Special constructors used to implement |None()| and |Some()| */
> +  Maybe(None) : mIsSome(false) { }
> +
> +  Maybe(detail::TemporarySomeValue<T> aSome)

This shouldn't be necessary if Some changes as described above.

@@ +164,5 @@
> +  Maybe(const Maybe& aOther)
> +    : mIsSome(false)
> +  {
> +    if (aOther.mIsSome) {
> +      emplace(*aOther);

Apropos of nothing, and not really to suggest a change, "emplace" is a really weird verb.

@@ +172,5 @@
> +  Maybe(Maybe&& aOther)
> +    : mIsSome(aOther.mIsSome)
> +  {
> +    if (aOther.mIsSome) {
> +      ::new (mStorage.addr()) T(Move(*aOther));

You need |aOther.mIsSome = false;| or else you'll double-destroy here.  Note that move-construction only leaves the input in a safely destructible state -- the destructor will still run at a later time, assuming correct memory handling.

@@ +187,5 @@
> +    }
> +    return *this;
> +  }
> +
> +  Maybe& operator=(const Maybe&& aOther)

You shouldn't have a |const| here, as the type of a move assignment operator takes non-const&&.  Generally const&& is largely useless; if you're ever typing it, you're probably making a mistake:

http://www.codesynthesis.com/~boris/blog/2012/07/24/const-rvalue-references/

@@ +189,5 @@
> +  }
> +
> +  Maybe& operator=(const Maybe&& aOther)
> +  {
> +    if (&aOther != this) {

Given the difficulty involved in even reaching the point of self-assignment of a temporary being possible, I think typically we've done

  MOZ_ASSERT(this != &aOther, "self-moves are prohibited");

and left it at that.

@@ +194,5 @@
> +      reset();
> +      if (aOther.mIsSome) {
> +        mIsSome = true;
> +        ::new (mStorage.addr()) T(Move(*aOther));
> +      }

You have the same |aOther.mIsSome = false;| issue in this method, too, that I note further down.  (const&& may have led you astray here, because it would have forbid this addition.)

@@ +205,5 @@
> +  bool isSome() const { return mIsSome; }
> +  bool isNone() const { return !mIsSome; }
> +
> +  /* Returns the contents of this Maybe<T> by value. Unsafe unless |isSome()|. */
> +  T val() const

The val/value change doesn't seem worth it just to lose typing two characters, to me.  Could we just use value, valueOr, &c.?

@@ +217,5 @@
> +   * default value provided, which can be either a T value or a function
> +   * returning a T.
> +   */
> +  template<typename D>
> +  T valOr(D aDefault) const

Should perfectly forward D here, and in all the other functions similar-ish to this.

@@ +238,5 @@
> +    return &asT();
> +  }
> +
> +  /* Returns the contents of this Maybe<T> by pointer. Unsafe unless |isSome()|. */
> +  T* ptr()

Move this above operator-> and make that call this.

@@ +258,5 @@
> +    }
> +    return detail::EvaluateFunctionOrValue(aDefault);
> +  }
> +
> +  const T* ptr() const

Same move above operator->.

@@ +268,5 @@
> +  template<typename D>
> +  const T* ptrOr(D aDefault) const
> +  {
> +    if (isSome()) {
> +      return &asT();

Let's use ptr() instead of &asT(), everywhere.  (Analogous to the ref/asT thing I mention later on.)

@@ +286,5 @@
> +    return asT();
> +  }
> +
> +  /* Returns the contents of this Maybe<T> by ref. Unsafe unless |isSome()|. */
> +  T& ref()

Above operator* and make that use this.

@@ +306,5 @@
> +    }
> +    return detail::EvaluateFunctionOrReference(aDefault);
> +  }
> +
> +  const T& ref() const

Above operator*, you know the drill.

@@ +323,5 @@
> +  }
> +
> +  /* If |isSome()|, runs the provided function on the contents of this Maybe. */
> +  template<typename F>
> +  void apply(F aFunc)

It seems to me there's not really a good reason to pass the function by value here.  We can imagine functors that are only move-constructible or so, so we probably should perfectly forward |aFunc| in all these methods.

(You can still call with aFunc(...) even with this change, I believe.)

@@ +326,5 @@
> +  template<typename F>
> +  void apply(F aFunc)
> +  {
> +    if (isSome()) {
> +      aFunc(ref());

Hmm, so you have if (isSome()) ...use ref()... here, other places you use asT().  Let's use ref() instead of asT() everywhere for consistency.

@@ +363,5 @@
> +  template<typename R>
> +  Maybe<R> map(R(*aFunc)(T&))
> +  {
> +    if (isSome()) {
> +      return Maybe<R>::some(aFunc(ref()));

I think you can do

  template<typename R, typename A>
  Maybe<R>
  map(R (*aFunc)(A&& a))
  {
    if (isSome()) {
      return Maybe<R>::some(aFunc(Forward<A>(ref())));
    }
    return Maybe<R>::none();
  }

to perfectly forward this, but I'm not testing that I typed it all out exactly correctly.

@@ +369,5 @@
> +    return Maybe<R>::none();
> +  }
> +
> +  template<typename R>
> +  Maybe<R> map(R(*aFunc)(const T&)) const

With the perfect forwarding above, this overload doesn't have to exist.

@@ +379,5 @@
> +  }
> +
> +  /* Variant of |map()| that takes an additional argument for the function. */
> +  template<typename R, typename FA, typename A>
> +  Maybe<R> map(R(*aFunc)(T&, FA), A&& aArg)

And this would be

  template<typename R, typename A1, typename A2>
  Maybe<R>
  map(R (*aFunc)(A1&&, A2&&), A2&& aArg) const
  {
    if (isSome()) {
      return Maybe<R>::some(aFunc(Forward<A1>(ref()), Forward<A2>(aArg)));
    }
    return Maybe<R>::none();
  }

if I didn't screw anything up, too.

@@ +388,5 @@
> +    return Maybe<R>::none();
> +  }
> +
> +  template<typename R, typename FA, typename A>
> +  Maybe<R> map(R(*aFunc)(const T&, FA), A&& aArg) const

And then this overload can die.

@@ +407,5 @@
> +  }
> +
> +  /*
> +   * Constructs a T value in-place in this Maybe<T>'s storage. The arguments
> +   * to |emplace()| are the parameters to T's constructor.

Could be worth s/this/this empty/ to be clear about the isNone() requirement.

@@ +416,5 @@
> +    ::new (mStorage.addr()) T();
> +    mIsSome = true;
> +  }
> +
> +  template<class T1>

typename for (all) these, since they might not be classes

@@ +460,4 @@
>    }
>  
>    template<class T1, class T2, class T3, class T4, class T5, class T6>
> +  void emplace(T1&& t1, T2&& t2, T3&& t3, T4&& t4, T5&& t5, T6&& t6) {

You're inconsistent about whether { goes on same line or a new line in the rest of these.  Be consistent.

@@ +537,5 @@
> + * Maybe<T> values are ordered in the same way T values are ordered, except that
> + * None comes before anything else.
> + */
> +template<typename T> bool
> +operator<(const Maybe<T>& aLHS, const Maybe<T>& aRHS)

Do we need this for hashing or sorting or something like that?  I have to say, comparability of Maybes seems kind of a stretch, as aside from the ordering of non-empty Maybe, it's not clear to me there's an obvious intuition about how one would order them.

Comment 42

[Jeff Walden [:Waldo]]

Oh, adding a Swap(Maybe&, Maybe&) overload probably makes sense, too, while we're here.  (At least, if the default Swap doesn't work, which on second thought maybe it does, not sure.)

Comment 43

[Seth Fowler [:seth] [:s2h]]

Thanks for the review, Jeff! I agreed with pretty much all of your comments above; I'll make those changes.

I'll take your advice and split this into two patches to simplify the process of reviewing and landing it.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #41)
> > +template<typename T> bool
> > +operator<(const Maybe<T>& aLHS, const Maybe<T>& aRHS)
> 
> Do we need this for hashing or sorting or something like that?  I have to
> say, comparability of Maybes seems kind of a stretch, as aside from the
> ordering of non-empty Maybe, it's not clear to me there's an obvious
> intuition about how one would order them.

We absolutely need this. I'm going to stick to my guns on this one. =) Without comparison operators, Maybe values won't be storable in many containers, and a lot of generic code won't apply to them. And equality/inequality end up getting used constantly. (Not sure if you meant to include those ones in this comment.)

The comparability of Maybe values also has plenty of precedent - Haskell, Scala, and Rust all provide it in much the same way I have here. Anyone familiar with any of those languages will find this very natural. And it's not really so unintuitive, if you think about it - placing None either before or after all Some values is the natural way to do it.

Comment 44

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Revamp Maybe<T> to be usable in many more situations

This updated version of part 1 should address all of the review comments so far. As suggested, I've split things up so that part 1 now only contains the changes to the API that affect parts 2-8. Part 9 will contain the remainder of the changes.

Comment 45

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 9) - Add more useful features to the Maybe<T> API

This part contains the rest of the API changes that were split out from the original part 1. All of the review comments so far should be addressed.

I've also added one minor addition: |ToMaybe()| allows you to convert a pointer to a Maybe value. We've been using the convention in many places in Gecko of using pointers as informal Maybe tpyes, with null meaning 'Nothing' and a non-null value meaning 'Some'. |ToMaybe()| allows such code to easily interface with code that does use Maybe, so that we can easily write new APIs that use Maybe and interact with old APIs without awkward conversions at the boundaries.

There isn't an equivalent of |ToMaybe()| for the other direction (converting a Maybe value to a pointer) because |maybeValue.ptrOr(nullptr)| seems sufficiently terse to be usable. (Though I'd be open to reconsidering; it wouldn't be hard to add another method that did that even more tersely.)

Comment 46

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 10) - Add a test for Maybe<T>

This patch adds a lot of tests for Maybe. (It tests both part 1 and part 9; I'd land this together with part 9.)

Comment 47

[Jeff Walden [:Waldo]]

Comment on attachment 8468715 [details] [diff] [review]
(Part 1) - Revamp Maybe<T> to be usable in many more situations

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

So there are a few nits, and two very substantial comments, that need addressing before this is good.  But the changes for all that are small, so this gets the conditional r+.

::: mfbt/Maybe.h
@@ +32,5 @@
> + *   void doSomething(Maybe<Foo> aFoo) {
> + *     if (aFoo)                  // Make sure that aFoo contains a value...
> + *       aFoo->takeAction();      // and then use |aFoo->| to access it.
> + *   }                            // |*aFoo| also works!
> + *  

Trailing whitespace.

@@ +108,4 @@
>    {
> +    if (aOther.mIsSome) {
> +      ::new (mStorage.addr()) T(Move(*aOther));
> +      aOther.mIsSome = false;

We're doing it subtly wrong here.  We should instead have:

  ::new (mStorage.addr()) T(Move(*aOther));
  aOther.reset();

As it is right now, by setting mIsSome to false, you're preventing the moved-but-still-requiring-destruction value from being destructed.  For a class like T = UniquePtr<int> that's not a big deal, because the undestructed object doesn't keep anything alive.  But in general, T can keep whatever it wants alive after being moved, so we have to be careful about that.  Here's a testcase demonstrating the problem:

struct Counted
{
  static size_t Created;
  static size_t Destroyed;
  const char* kind;

  Counted() : kind("by ()") { printf("create (): %p\n", (void*)this); Created++; }
  Counted(Counted&&) : kind("by (&&)") { printf("create (&&): %p\n", (void*)this); Created++; }
  ~Counted() { printf("destroy (kind %s): %p\n", kind, (void*)this); Destroyed++; }
};

/* static */ size_t Counted::Created = 0;
/* static */ size_t Counted::Destroyed = 0;

static void TestMaybe()
{
  {
    printf("first line\n");
    auto s(mozilla::Some(Counted()));
    printf("second line\n");
    mozilla::Maybe<Counted> maybe(mozilla::Move(s));
    printf("before destruction\n");
  }
  MOZ_ASSERT(Counted::Created == Counted::Destroyed,
             "failed to properly destroy all Counted instances");
}

(Note that |s| is necessary, as if you just pass |mozilla::Some(Counted())| directly into |maybe| the compiler may well elide a copy/move on you and the assertion won't fail.)

Here's behavior without this change, and with it, for illustration:

[jwalden@find-waldo-now src]$ dbg/mfbt/tests/TestArrayUtils 
first line
create (): 0x7fff5b8325c8
create (&&): 0x7fff5b8325d8
destroy (kind by ()): 0x7fff5b8325c8
second line
create (&&): 0x7fff5b8325c0
before destruction
destroy (kind by (&&)): 0x7fff5b8325c0
Assertion failure: Counted::Created == Counted::Destroyed (failed to properly destroy all Counted instances), at /home/jwalden/moz/slots/mfbt/tests/TestArrayUtils.cpp:328
Segmentation fault (core dumped)

[jwalden@find-waldo-now src]$ dbg/mfbt/tests/TestArrayUtils 
first line
create (): 0x7fff07eec8a8
create (&&): 0x7fff07eec8b8
destroy (kind by ()): 0x7fff07eec8a8
second line
create (&&): 0x7fff07eec8a0
destroy (kind by (&&)): 0x7fff07eec8b8
before destruction
destroy (kind by (&&)): 0x7fff07eec8a0

@@ +137,5 @@
> +        ref() = Move(aOther.ref());
> +      } else {
> +        mIsSome = true;
> +        ::new (mStorage.addr()) T(Move(*aOther));
> +      }

Need an |aOther.reset()| after this.  Same reasons as the other case.  Minimally-modified version of the previous test for this:

struct Counted
{
  static size_t Created;
  static size_t Destroyed;
  const char* kind;

  Counted() : kind("by ()") { printf("create (): %p\n", (void*)this); Created++; }
  Counted(Counted&&) : kind("by (&&)") { printf("create (&&): %p\n", (void*)this); Created++; }
  ~Counted() { printf("destroy (kind %s): %p\n", kind, (void*)this); Destroyed++; }
  Counted& operator=(Counted&&) {
    kind = "assigned";
    return *this;
  }
};

/* static */ size_t Counted::Created = 0;
/* static */ size_t Counted::Destroyed = 0;

static void TestMaybe()
{
  {
    printf("first line\n");
    auto s(mozilla::Some(Counted()));
    printf("second line\n");
    mozilla::Maybe<Counted> maybe;
    printf("third line\n");
    maybe = mozilla::Move(s);
    printf("before destruction\n");
  }
  MOZ_ASSERT(Counted::Created == Counted::Destroyed,
             "failed to properly destroy all Counted instances");
}

With pre-resetting output, post-resetting output:

[jwalden@find-waldo-now src]$ dbg/mfbt/tests/TestArrayUtils 
first line
create (): 0x7fff430aa468
create (&&): 0x7fff430aa478
destroy (kind by ()): 0x7fff430aa468
second line
third line
create (&&): 0x7fff430aa460
before destruction
destroy (kind by (&&)): 0x7fff430aa460
Assertion failure: Counted::Created == Counted::Destroyed (failed to properly destroy all Counted instances), at /home/jwalden/moz/slots/mfbt/tests/TestArrayUtils.cpp:334
Segmentation fault (core dumped)

[jwalden@find-waldo-now src]$ dbg/mfbt/tests/TestArrayUtils 
first line
create (): 0x7ffff16f7458
create (&&): 0x7ffff16f7468
destroy (kind by ()): 0x7ffff16f7458
second line
third line
create (&&): 0x7ffff16f7450
destroy (kind by (&&)): 0x7ffff16f7468
before destruction
destroy (kind by (&&)): 0x7ffff16f7450

@@ +142,5 @@
> +    } else {
> +      reset();
> +    }
> +
> +    aOther.mIsSome = false;

And with the resetting required above, this line is no longer necessary/desired.

@@ +148,4 @@
>    }
>  
> +  /* Methods that check whether this Maybe contains a value */
> +  operator ConvertibleToBool() const { return mIsSome ? & Maybe::nonNull : 0; }

& directly next to function, please.

@@ +219,5 @@
>    }
>  
> +  /*
> +   * Constructs a T value in-place in this empty Maybe<T>'s storage. The
> +   * arguments to |emplace()| are the parameters to T's constructor.

Perhaps worth the omnipresent note that you can't pass literal nullptr to these functions without hitting b2g-only compile errors.

@@ +332,4 @@
>  };
>  
> +template<typename T>
> +Maybe<typename RemoveCV<typename RemoveReference<T>::Type>::Type>

So Maybe can never be a nothing-or-reference structure, at least if you want to use Some() to create it?  Okay.  This seems worth discussing in a documentation comment here.

Comment 48

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Revamp Maybe<T> to be usable in many more situations

Rebased part 1 and took care of the remaining build issues.

Comment 49

[Seth Fowler [:seth] [:s2h]]

(And addressed review comments, of course.)

Try looks green, so I went ahead and pushed parts 1-8.

https://hg.mozilla.org/integration/mozilla-inbound/rev/834c6dadc1b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca69de269066
https://hg.mozilla.org/integration/mozilla-inbound/rev/abad9c2ddec8
https://hg.mozilla.org/integration/mozilla-inbound/rev/69d0a773505e
https://hg.mozilla.org/integration/mozilla-inbound/rev/59b1637dc740
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f2f47600849
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d08e60f8991
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbc2ccf36d2e

Comment 50

[[e-mail]]

Comment on attachment 8472694 [details] [diff] [review]
(Part 1) - Revamp Maybe<T> to be usable in many more situations

>+  Maybe(Maybe&& aOther)
>+    : mIsSome(aOther.mIsSome)
>   {
>+    if (aOther.mIsSome) {
>+      ::new (mStorage.addr()) T(Move(*aOther));
>+      aOther.reset();
>+    }
>   }
[I want to suggest emplace here to reduce copy/paste of ::new, but it's nontrivial.]

>+  Maybe& operator=(Maybe&& aOther)
>   {
>+    MOZ_ASSERT(this != &aOther, "Self-moves are prohibited");
>+
>+    if (aOther.mIsSome) {
>+      if (mIsSome) {
>+        ref() = Move(aOther.ref());
>+      } else {
>+        mIsSome = true;
>+        ::new (mStorage.addr()) T(Move(*aOther));
emplace?

Comment 51

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/834c6dadc1b2
https://hg.mozilla.org/mozilla-central/rev/ca69de269066
https://hg.mozilla.org/mozilla-central/rev/abad9c2ddec8
https://hg.mozilla.org/mozilla-central/rev/69d0a773505e
https://hg.mozilla.org/mozilla-central/rev/59b1637dc740
https://hg.mozilla.org/mozilla-central/rev/0f2f47600849
https://hg.mozilla.org/mozilla-central/rev/3d08e60f8991
https://hg.mozilla.org/mozilla-central/rev/bbc2ccf36d2e

Comment 52

[Jonathan Kew [:jfkthame]]

FTR, this appears to have broken my local build (on Mac OS X 10.7.5, clang from MacPorts, debug build of Gecko); it dies with an internal compiler error:

28:09.89 Invalid operator call kind
28:09.89 UNREACHABLE executed at StmtProfile.cpp:566!
28:09.94 0  libLLVM-3.4.dylib 0x0000000103690936 llvm::sys::PrintStackTrace(__sFILE*) + 40
28:09.94 1  libLLVM-3.4.dylib 0x0000000103690d3d _ZL13SignalHandleri + 241
28:09.94 2  libsystem_c.dylib 0x00007fff85ca9cfa _sigtramp + 26
28:09.94 3  libsystem_c.dylib 000000000000000000 _sigtramp + 2050319136
28:09.94 4  libLLVM-3.4.dylib 0x0000000103690bb5 abort + 22
28:09.94 5  libLLVM-3.4.dylib 0x0000000103681f2e LLVMInstallFatalErrorHandler + 0
28:09.94 6  clang             0x00000001020b35a7 clang::StmtVisitorBase<clang::make_const_ptr, (anonymous namespace)::StmtProfiler, void>::Visit(clang::Stmt const*) + 4851
28:09.94 7  clang             0x00000001020b3db9 (anonymous namespace)::StmtProfiler::VisitStmt(clang::Stmt const*) + 153
28:09.94 8  clang             0x00000001020b2e36 clang::StmtVisitorBase<clang::make_const_ptr, (anonymous namespace)::StmtProfiler, void>::Visit(clang::Stmt const*) + 2946
28:09.94 9  clang             0x00000001020b3db9 (anonymous namespace)::StmtProfiler::VisitStmt(clang::Stmt const*) + 153
28:09.94 10 clang             0x00000001020b239c clang::StmtVisitorBase<clang::make_const_ptr, (anonymous namespace)::StmtProfiler, void>::Visit(clang::Stmt const*) + 232
28:09.94 11 clang             0x00000001020b3db9 (anonymous namespace)::StmtProfiler::VisitStmt(clang::Stmt const*) + 153
28:09.94 12 clang             0x00000001020b23c0 clang::StmtVisitorBase<clang::make_const_ptr, (anonymous namespace)::StmtProfiler, void>::Visit(clang::Stmt const*) + 268
28:09.94 13 clang             0x00000001020b22ae clang::Stmt::Profile(llvm::FoldingSetNodeID&, clang::ASTContext const&, bool) const + 34
<...and many more frames...>

28:10.00 0.	Program arguments: /opt/local/libexec/llvm-3.4/bin/clang -cc1 -triple x86_64-apple-macosx10.6.0 -emit-obj -mrelax-all -disable-free -main-file-name RegExp.cpp -mrelocation-model pic -pic-level 2 -mdisable-fp-elim -masm-verbose -munwind-tables -target-cpu core2 -target-linker-version 236.3 -gdwarf-2 -coverage-file /Users/jkew/mozdev/mc-clean/obj-ff-dbg/js/src/RegExp.o -resource-dir /opt/local/libexec/llvm-3.4/bin/../lib/clang/3.4.2 -dependency-file .deps/RegExp.o.pp -MT RegExp.o -sys-header-deps -MP -include ../../js/src/js-confdefs.h -D FFI_BUILDING -D ENABLE_PARALLEL_JS -D ENABLE_BINARYDATA -D ENABLE_SHARED_ARRAY_BUFFER -D JSGC_FJGENERATIONAL -D EXPORT_JS_API -D JS_HAS_CTYPES -D DLL_PREFIX="lib" -D DLL_SUFFIX=".dylib" -D USE_SYSTEM_MALLOC=1 -D ENABLE_ASSEMBLER=1 -D AB_CD= -D NO_NSPR_10_SUPPORT -D MOZILLA_CLIENT -D DEBUG -D TRACING -I /Users/jkew/mozdev/mc-clean/js/src -I . -I /Users/jkew/mozdev/mc-clean/js/src/../../mfbt/double-conversion -I ctypes/libffi/include -I /Users/jkew/mozdev/mc-clean/intl/icu/source/common -I /Users/jkew/mozdev/mc-clean/intl/icu/source/i18n -I ../../dist/include -I /Users/jkew/mozdev/mc-clean/obj-ff-dbg/dist/include/nspr -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Werror=int-to-pointer-cast -Wtype-limits -Wempty-body -Werror=conversion-null -Wsign-compare -Wno-invalid-offsetof -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -std=gnu++0x -fdeprecated-macro -fdebug-compilation-dir /Users/jkew/mozdev/mc-clean/obj-ff-dbg/js/src -ferror-limit 19 -fmessage-length 0 -fvisibility hidden -pthread -stack-protector 1 -mstackrealign -fblocks -fno-rtti -fobjc-runtime=macosx-10.6.0 -fencode-extended-block-signature -fno-common -fdiagnostics-show-option -vectorize-slp -o RegExp.o -x c++ /Users/jkew/mozdev/mc-clean/js/src/builtin/RegExp.cpp


Obviously, this isn't happening in automation, so it's some kind of local issue; time to try updating my compiler, I suppose. Unfortunately, MacPorts thinks what I have is current, so I'll have to try some other approach.

(Tried the MacPorts versions of clang 3.3, 3.4.2, and 3.6, and all three fail similarly.)

Comment 53

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 9) - Add an implementation of declval

I'm uploading the versions of parts 9, 10, and 11 that I would've used if not for a bug in GCC 4.4. If we ever actually manage to get off of GCC 4.4 on B2G, these versions nicely support functors for map() and apply().

Comment 54

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 10) - Add more useful features to the Maybe<T> API

Again, this version can't be used until we move off of GCC 4.4.

Comment 55

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 11) - Add a test for Maybe<T>

This version of the tests contains tests for the functor code that won't work with GCC 4.4.

Comment 56

[Seth Fowler [:seth] [:s2h]]

(In reply to Jonathan Kew (:jfkthame) from comment #52)
> FTR, this appears to have broken my local build (on Mac OS X 10.7.5, clang
> from MacPorts, debug build of Gecko); it dies with an internal compiler

Not sure why that's happening. I can't reproduce that here, but I don't use OS X 10.7 or MacPorts (I install clang from Homebrew). That's an internal compiler error... pretty nasty.

Comment 57

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 9) - Add more useful features to the Maybe<T> API

Alright, I switched |map()| back to supporting function pointers only, and gave up on perfect forwarding for the function's arguments for the time being. My implementation was tripping a GCC 4.4 compiler bug that there doesn't seem to be an easy workaround for. I'll address these issues as a followup in bug 1054115, but for now I think it's better to get this code in the tree and unstick the things that are waiting on it.

Comment 58

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 10) - Add a test for Maybe<T>

Here are the updated tests. Unfortunately a few are commented out; those will get turned back on in the followups.

Comment 59

[Seth Fowler [:seth] [:s2h]]

Note by the way that the new part 9 also incorporates Neil's suggestion in comment 50 to use |emplace()| in the move assignment operator implementation.

Comment 60

[Seth Fowler [:seth] [:s2h]]

Here's a try job for the current version of the patch:

https://tbpl.mozilla.org/?tree=Try&rev=33757459800f

Comment 61

[Jonathan Kew [:jfkthame]]

(In reply to Seth Fowler [:seth] from comment #56)
> (In reply to Jonathan Kew (:jfkthame) from comment #52)
> > FTR, this appears to have broken my local build (on Mac OS X 10.7.5, clang
> > from MacPorts, debug build of Gecko); it dies with an internal compiler
> 
> Not sure why that's happening. I can't reproduce that here, but I don't use
> OS X 10.7 or MacPorts (I install clang from Homebrew). That's an internal
> compiler error... pretty nasty.

What clang version, exactly? I tried getting clang from homebrew, but it still dies with a similar internal error here. :(

Comment 62

[Seth Fowler [:seth] [:s2h]]

(In reply to Jonathan Kew (:jfkthame) from comment #61)
> What clang version, exactly? I tried getting clang from homebrew, but it
> still dies with a similar internal error here. :(

We talked about this on IRC, but for the benefit of anyone else who may run into this, I'm running "Apple LLVM version 5.1 (clang-503.0.40) (based on LLVM 3.4svn)" on OS X 10.9.

Comment 63

[Seth Fowler [:seth] [:s2h]]

I made a new bug to discuss the issue that Jonathan is seeing: bug 1054394.

Comment 64

[Jeff Walden [:Waldo]]

Comment on attachment 8473527 [details] [diff] [review]
(Part 9) - Add more useful features to the Maybe<T> API

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

Make sure to coalesce this into one revision with the tests.

::: mfbt/Maybe.h
@@ +173,5 @@
> +  {
> +    if (isSome()) {
> +      return ref();
> +    }
> +    return aDefault;

This should be Forward<V>(aDefault) so that a passed-in temporary is move-copied rather than incurring an extra copy.

@@ +206,5 @@
> +  /*
> +   * Returns the contents of this Maybe<T> by pointer. If |isNothing()|,
> +   * returns the default value provided.
> +   */
> +  T* ptrOr(T* aDefault)

Probably should be pointerOr for consistency with valueOr.

@@ +227,5 @@
> +   * Returns the contents of this Maybe<T> by pointer. If |isNothing()|,
> +   * returns the value returned from the function or functor provided.
> +   */
> +  template<typename F>
> +  T* ptrOrFrom(F&& aFunc)

And pointerOrFrom.

@@ +584,5 @@
> +}
> +
> +/*
> + * We support comparison to Nothing to allow reasonable expressions like:
> + *   if (maybeValue == Nothing()) { ... }

Provide the symmetric operators, too.

Comment 65

[Jeff Walden [:Waldo]]

Comment on attachment 8473528 [details] [diff] [review]
(Part 10) - Add a test for Maybe<T>

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

::: mfbt/tests/TestMaybe.cpp
@@ +24,5 @@
> +
> +// Work around a bug in Visual Studio 2010 that prevents expressions of the form
> +// |decltype(foo)::type| from working. See here:
> +// http://stackoverflow.com/questions/14330768/c11-compiler-error-when-using-decltypevar-followed-by-internal-type-of-var
> +#if _MSC_VER <= 1600

There's a compiler-testing macro in mozilla/Compiler.h that makes clear what "1600" means (by not using that number), use that here.

@@ +34,5 @@
> +
> +#define CHECK(c) \
> +  do { \
> +    bool cond = (c); \
> +    MOZ_ASSERT(cond, "Failed assertion: " #c); \

Just use MOZ_RELEASE_ASSERT instead of this whole CHECK thing, I think.

@@ +45,5 @@
> +    bool cond = (t()); \
> +    if (!cond) \
> +      return 1; \
> +    cond = AllDestructorsWereCalled(); \
> +    MOZ_ASSERT(cond, "Failed to destroy all objects during test: " #t); \

MOZ_RELEASE_ASSERT, throughout, actually.

@@ +61,5 @@
> +  eWasMoveAssigned,
> +  eWasMovedFrom
> +};
> +
> +static unsigned sUndestroyedObjects = 0;

size_t, you're counting stuff.

@@ +79,5 @@
> +  }
> +
> +  explicit BasicValue(int aTag)
> +    : mStatus(eWasConstructed)
> +      , mTag(aTag)

Misaligned.

@@ +165,5 @@
> +  Status GetStatus() { return mStatus; }
> +
> +private:
> +  UncopyableValue(const UncopyableValue& aOther);
> +  UncopyableValue& operator=(const UncopyableValue& aOther);

Put MOZ_DELETEs on these if you can.

@@ +196,5 @@
> +  Status GetStatus() { return mStatus; }
> +
> +private:
> +  UnmovableValue(UnmovableValue&& aOther);
> +  UnmovableValue& operator=(UnmovableValue&& aOther);

MOZ_DELETE again

@@ +223,5 @@
> +private:
> +  UncopyableUnmovableValue(const UncopyableUnmovableValue& aOther);
> +  UncopyableUnmovableValue& operator=(const UncopyableUnmovableValue& aOther);
> +  UncopyableUnmovableValue(UncopyableUnmovableValue&& aOther);
> +  UncopyableUnmovableValue& operator=(UncopyableUnmovableValue&& aOther);

MOZ_DELETE

@@ +320,5 @@
> +  CHECK(mayBasicValue->GetTag() == 3);
> +
> +  // Check that we get copies when moves aren't possible.
> +  Maybe<BasicValue> mayBasicValue2 = Some(*mayBasicValue);
> +  CHECK(mayBasicValue2->GetStatus() == eWasCopyConstructed);

I...think this might be relying on C++-optional assignment elision here.  Maybe.  Details hazy in memory.  Would somewhat prefer (Some(...)) or something that doesn't trigger that possibility.

@@ +546,5 @@
> +static void
> +AccessTag(const BasicValue& aValue)
> +{
> +  gFunctionWasApplied = true;
> +  aValue.GetTag();

Maybe write this into a pointer to extern global or something, so that the compiler has a harder time optimizing away this access?  I expect it would do so now.

@@ +617,5 @@
> +  CHECK(gFunctionWasApplied);
> +  gFunctionWasApplied = false;
> +  mayValueCRef.apply(&AccessTagWithArg, 1);
> +  CHECK(gFunctionWasApplied);
> +  

Trailing whitespace.

@@ +776,5 @@
> +
> +static bool
> +TestComparisonOperators()
> +{
> +  using namespace std::rel_ops;

I don't think you're using this any more here, and if you were, stlport, I assume.

Comment 66

[Seth Fowler [:seth] [:s2h]]

Thanks for the reviews! This really made my day!

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #64)
> Probably should be pointerOr for consistency with valueOr.

I'm not keen on making this more verbose, but I don't feel strongly about it. If we want to do this, though, let's do this in a followup, because this affects the parts that have already landed on central.

Comment 67

[Seth Fowler [:seth] [:s2h]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #65)
> > +  // Check that we get copies when moves aren't possible.
> > +  Maybe<BasicValue> mayBasicValue2 = Some(*mayBasicValue);
> > +  CHECK(mayBasicValue2->GetStatus() == eWasCopyConstructed);
> 
> I...think this might be relying on C++-optional assignment elision here. 
> Maybe.  Details hazy in memory.  Would somewhat prefer (Some(...)) or
> something that doesn't trigger that possibility.

I could be wrong, but I think this is OK. The copy construction happens inside |Maybe<BasicValue>(const Maybe& aOther)|, which gets called because we're initializing mayBasicValue2 with another Maybe<BasicValue>. (We're copy-initializing, in other words.) Inside |Maybe<BasicValue>(const Maybe& aOther)|, the call to |emplace()| ends up invoking the copy constructor of BasicValue.

Comment 68

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 9) - Add more useful features to the Maybe<T> API

Thanks again for the excellent reviews, Jeff. Here's a new version of part 9 that takes your review comments into account.

I pushed a try job that contains both this patch and part 10, and it's all green:

https://tbpl.mozilla.org/?tree=Try&rev=4456f0e1cd02

Comment 69

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 10) - Add a test for Maybe<T>

Here's the revised version of part 10. (Also included in the try job above.)

Comment 70

[Seth Fowler [:seth] [:s2h]]

Unfortunately inbound has gone down in flames and I can't stay up any later, but I'm marking this 'checkin-needed' in the hope that some kind soul will check it in for me when inbound reopens. =)

(Everything but parts 9 and 10 has already been checked in. Jeff asked that parts 9 and 10 be folded into one commit for checkin.)

Comment 71

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7283263a1974

Comment 72

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7283263a1974

Comment 73

[Seth Fowler [:seth] [:s2h]]

Alright, looks like all parts have hit m-c. I'm resolving this.