1185706 - Add support to Tie() for Pair

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

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

Let's add support to Tie() for Pair. In the STL this just works because std::tuple has operator=(const pair<T,U>&). We probably want to do the same.

Comment 1

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

We actually probably want mozilla::Tie() to support both mozilla::Pair and std::pair, since both are used in our codebase.

Comment 2

[Gary Chen [:xeonchen]]

Attachment: 0001-Bug-1185706-support-Tie-for-mozilla-Pair-r-froydnj.patch

mozilla::Pair and std::pair are supported by specialize a 2-tuple implementation.

@seth: do you think this is what you want?

Comment 3

[Botond Ballo [:botond]]

Comment on attachment 8637070 [details] [diff] [review]
0001-Bug-1185706-support-Tie-for-mozilla-Pair-r-froydnj.patch

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

I think this specialization approach will work (this is what libstdc++ does). However, I don't think you need the conversion constructor / assignment operator of the two-arg specialization to be variadic, since we know the exact number (two) of element we want.

Comment 4

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

Comment on attachment 8637070 [details] [diff] [review]
0001-Bug-1185706-support-Tie-for-mozilla-Pair-r-froydnj.patch

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

Yeah, this seems like it'll do the job nicely. Thanks!

Comment 5

[Gary Chen [:xeonchen]]

Attachment: 0001-Bug-1185706-support-Tie-for-mozilla-Pair-r-botond.patch

Remove variadic parameters on 2-arg specialization.
Use explicit constructor to pass static checking.
Fix TestTuple.c compilation error on some platforms.

Comment 6

[Gary Chen [:xeonchen]]

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2642c5912fe2

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8638248 [details] [diff] [review]
0001-Bug-1185706-support-Tie-for-mozilla-Pair-r-botond.patch

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

LGTM (with comments addressed). I think the official nod needs to come from Nathan, though.

::: mfbt/Tuple.h
@@ +249,5 @@
>  };
>  
>  /**
> + * Specialization of Tuple for two elements.
> + * This is created to support assignment from a Pair or std::pair.

s/assignment/construction and assignment

@@ +265,5 @@
> +  explicit Tuple(const A& aA, const B& aB) : Impl(aA, aB) { }
> +  // Here, we can't just use 'typename OtherElements' because MSVC will give
> +  // a warning "C4520: multiple default constructors specified" (even if no one
> +  // actually instantiates the constructor with an empty parameter pack -
> +  // that's probably a bug) and we compile with warnings-as-errors.

This comment is only relevant to the variadic case. It can be removed in the specialization.

@@ +266,5 @@
> +  // Here, we can't just use 'typename OtherElements' because MSVC will give
> +  // a warning "C4520: multiple default constructors specified" (even if no one
> +  // actually instantiates the constructor with an empty parameter pack -
> +  // that's probably a bug) and we compile with warnings-as-errors.
> +  template <typename OtherHead, typename OtherTail,

Head/Tail implies a list - I think OtherA and OtherB would be better names. Or even better, AArg and BArg, as that's what you use below.

@@ +311,5 @@
> +  Tuple& operator=(const Pair<AArg, BArg>& aOther)
> +  {
> +    this->Head(static_cast<Impl&>(*this)) = aOther.first();
> +    this->Tail(static_cast<Impl&>(*this)).Head(static_cast<Impl&>(*this)) =
> +      aOther.second();

I think these calls can be made a little more concise (haven't tested it though):

Impl::Head(*this) = aOther.first();
Impl::Tail(*this).Head(*this) = aOther.second();

Likewise in the other three operator= overloads below.

::: mfbt/tests/TestTuple.cpp
@@ +197,5 @@
> +  Tuple<UniquePtr<int>, UniquePtr<int>> e{MakeUnique<int>(0), MakeUnique<int>(0)};
> +  // XXX: On some platforms std::pair doesn't support move constructor.
> +  pair<UniquePtr<int>, UniquePtr<int>> f;
> +  f.first = Move(MakeUnique<int>(42));
> +  f.second = Move(MakeUnique<int>(42));

MakeUnique<int>(42) is already an rvalue, there should be no need to Move() it.

Comment 8

[Gary Chen [:xeonchen]]

Attachment: 0001-Bug-1185706-support-Tie-for-mozilla-Pair-r-froydnj.patch

Comment 9

[Gary Chen [:xeonchen]]

(In reply to Botond Ballo [:botond] from comment #7)
> Comment on attachment 8638248 [details] [diff] [review]
> 0001-Bug-1185706-support-Tie-for-mozilla-Pair-r-botond.patch
> 
> Review of attachment 8638248 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM (with comments addressed). I think the official nod needs to come from
> Nathan, though.
> 

Thank you Botond!

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8638401 [details] [diff] [review]
0001-Bug-1185706-support-Tie-for-mozilla-Pair-r-froydnj.patch

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

I'm not a huge fan of having to include <utility> to support std::pair here; we've generally tried to avoid std:: stuff in MFBT.

However, I think the argument here would be that we *don't* have std::tie available in all our C++ libraries, so polyfilling std::tie for std::pair here is OK.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7dee2347f114

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7dee2347f114