PSA: mozilla::Pair is now a little more flexible

[Seth Fowler]

I thought I’d let everyone know that bug 1142366 and bug 1142376 have added some handy new features to mozilla::Pair. In particular:

- Pair objects are now movable. (It’s now a requirement that the underlying types be movable too. Every existing use satisfied this requirement.)

- Pair objects are now copyable if the underlying types are copyable.

- We now have an equivalent of std::make_pair, mozilla::MakePair. This lets you construct a Pair object with type inference. So this code:

> Pair<Foo, Bar> GetPair() {
> return Pair<Foo, Bar>(Foo(), Bar());
> }

Becomes:

> Pair<Foo, Bar> GetPair() {
> return MakePair(Foo(), Bar());
> }

Nice! This can really make a big difference for long type names or types which have their own template parameters.

These changes should make Pair a little more practical to use. Enjoy!

- Seth

[Eric Rescorla]

Sorry if this is a dumb question, but it seems like std::pair is fairly
widely used in our
code base. Can you explain the circumstances in which you think we should be
using mozilla::Pair instead?

Ekr

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[Seth Fowler]

> On Mar 13, 2015, at 6:14 AM, Eric Rescorla <e...@rtfm.com> wrote:
>
> Sorry if this is a dumb question, but it seems like std::pair is fairly widely used in our
> code base. Can you explain the circumstances in which you think we should be
> using mozilla::Pair instead?

It’s not at all a dumb question. This came up on IRC every time mozilla::Pair came up, so a lot of people are wondering about this.

I’m not the person that originally introduced mozilla::Pair, so I wouldn’t consider my answer to this question definitive, but I’ll give it a shot anyway.

mozilla::Pair is about avoiding implementation quality issues with std::pair. There are two quality issues in particular that have bit us in the past:

- Poor packing, particularly when one of the types stored in the pair has no members. In that situation the empty type should consume no space, but std::pair implementations sometimes don’t handle that case efficiently.

- Poor or non-existent support for move semantics. I don’t know specifically about the case of std::pair, but this is still biting people with other STL containers quite recently. Obviously the same code can have significantly different performance characteristics in some cases depending on move semantics support, so this is a serious problem.

Until we know that we can rely on high quality std::pair implementations everywhere, my recommendation would be to always use mozilla::Pair.

- Seth

[Eric Rescorla]

I'm not sure I want to get in a long argument about this, but I'm not
convinced
this is good advice.

Just looking at dxr shows a really large number of uses of std::pair,
especially in
pieces of code that we don't control, and no uses of mozilla::Pair() other
than those in the
test code and mfbt itself [0] So I would suggest that we've already largely
incurred
those costs, whatever they are. Which platforms do you believe have these
issues?

Given the current situation, and absent some evidence that this is actually
causing
real problems in the field, it seems like there's a huge amount of benefit
in using
standard constructs.

-Ekr

[0]
https://dxr.mozilla.org/mozilla-central/search?q=%2Btype-ref%3Amozilla%3A%3APair

[Seth Fowler]

> On Mar 15, 2015, at 12:01 PM, Eric Rescorla <e...@rtfm.com> wrote:
>
> I'm not sure I want to get in a long argument about this, but I'm not convinced
> this is good advice.

I don’t really care what we do - keep in mind, I had nothing to do with introducing mozilla::Pair - but I think that we should recommend the use of one thing, either std::pair or mozilla::Pair. If we choose to prefer std::pair, we should probably remove mozilla::Pair.

If you’re convinced that you have a good case for standardizing on std::pair, please file a bug about removing mozilla::Pair, and everyone interested in the matter can have a technical discussion about it there.

- Seth

[Eric Rescorla]

On Sun, Mar 15, 2015 at 12:33 PM, Seth Fowler <se...@mozilla.com> wrote:

>
> > On Mar 15, 2015, at 12:01 PM, Eric Rescorla <e...@rtfm.com> wrote:
> >
> > I'm not sure I want to get in a long argument about this, but I'm not
> convinced
> > this is good advice.
>
> I don’t really care what we do - keep in mind, I had nothing to do with
> introducing mozilla::Pair - but I think that we should recommend the use of
> one thing, either std::pair or mozilla::Pair. If we choose to prefer
> std::pair, we should probably remove mozilla::Pair

Yes, I generally agree with this. But I'm also not interested in burning a
lot of
time on this as long as there's not some effort to stop people using
std::pair.

If you’re convinced that you have a good case for standardizing on
> std::pair, please file a bug about removing mozilla::Pair, and everyone
> interested in the matter can have a technical discussion about it there.

https://bugzilla.mozilla.org/show_bug.cgi?id=1143478

-Ekr

[Joshua Cranmer 🐧]

On 3/15/2015 2:33 PM, Seth Fowler wrote:
> I don’t really care what we do - keep in mind, I had nothing to do > with introducing mozilla::Pair - but I think that we should
recommend > the use of one thing, either std::pair or mozilla::Pair. If
we choose > to prefer std::pair, we should probably remove mozilla::Pair.

The reason why we have mozilla::Pair is that we needed a pair type that
was sizeof(T1) if T2 was empty (for mozilla::UniquePtr). I suggested
that such a utility might be more widely valuable and thus that it
should be split out as a separate mozilla:: type rather than a
mozillla::detail:: type. std::pair is required to have the two elements
be listed as members by the specification, although I think std::tuple
may similarly have the empty-types-take-no-space optimization
(mozilla::Pair was added before MSVC 2013 requirement and thus before
variadic templates).

In general, std::pair should be preferred over mozilla::Pair unless you
need the empty type optimization.

--
Joshua Cranmer
Thunderbird and DXR developer
Source code archæologist

[Seth Fowler]

> On Mar 15, 2015, at 6:26 PM, Joshua Cranmer 🐧 <Pidg...@gmail.com> wrote:
> In general, std::pair should be preferred over mozilla::Pair unless you need the empty type optimization.

If that’s the case, perhaps we should rename it to e.g. mozilla::CompactPair? It’s current name strongly suggests that it should serve as a replacement for std::pair.

- Seth

[Ehsan Akhgari]

On 2015-03-15 11:26 PM, Seth Fowler wrote:
>
>> On Mar 15, 2015, at 6:26 PM, Joshua Cranmer 🐧 <Pidg...@gmail.com> wrote:

>> In general, std::pair should be preferred over mozilla::Pair unless you need the empty type optimization.
>

> If that’s the case, perhaps we should rename it to e.g. mozilla::CompactPair? It’s current name strongly suggests that it should serve as a replacement for std::pair.

Sounds like a good idea.

[Eric Rescorla]

On Mon, Mar 16, 2015 at 5:44 AM, Ehsan Akhgari <ehsan....@gmail.com>
wrote:

> On 2015-03-15 11:26 PM, Seth Fowler wrote:
>
>>

>> On Mar 15, 2015, at 6:26 PM, Joshua Cranmer [image: 🐧] <

>>> Pidg...@gmail.com> wrote:
>>> In general, std::pair should be preferred over mozilla::Pair unless you
>>> need the empty type optimization.
>>>
>>

>> If that’s the case, perhaps we should rename it to e.g.
>> mozilla::CompactPair? It’s current name strongly suggests that it should
>> serve as a replacement for std::pair.
>>
>
> Sounds like a good idea.

Given that we've been living with this in our code so far (except for
UniquePtr),
and that we'll get this when tuple becomes usable, this seems like a rather
temporary benefit at the cost of baking in yet another mozilla-specific
non-STLism.

My proposal would be instead to make this an inner class and encourage
people to use tuple when it becomes available.

-Ekr