Re: PSA: mozilla::Pair is now a little more flexible
On Mon, Mar 16, 2015 at 5:44 AM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: On 2015-03-15 11:26 PM, Seth Fowler wrote: On Mar 15, 2015, at 6:26 PM, Joshua Cranmer [image: ] pidgeo...@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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: PSA: mozilla::Pair is now a little more flexible
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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: PSA: mozilla::Pair is now a little more flexible
On Sun, Mar 15, 2015 at 12:33 PM, Seth Fowler s...@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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: PSA: mozilla::Pair is now a little more flexible
On Fri, Mar 13, 2015 at 10:54 AM, Seth Fowler s...@mozilla.com wrote: 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. 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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: PSA: mozilla::Pair is now a little more flexible
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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: PSA: mozilla::Pair is now a little more flexible
On Mar 15, 2015, at 6:26 PM, Joshua Cranmer pidgeo...@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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: PSA: mozilla::Pair is now a little more flexible
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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: PSA: mozilla::Pair is now a little more flexible
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
On Thu, Mar 12, 2015 at 5:53 PM, Seth Fowler s...@mozilla.com wrote:
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:
PairFoo, Bar GetPair() {
return PairFoo, Bar(Foo(), Bar());
}
Becomes:
PairFoo, 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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
PSA: mozilla::Pair is now a little more flexible
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:
PairFoo, Bar GetPair() {
return PairFoo, Bar(Foo(), Bar());
}
Becomes:
PairFoo, 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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
