Closed Bug 1185706 Opened 5 years ago Closed 5 years ago

Add support to Tie() for Pair

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: seth, Assigned: xeonchen)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
We actually probably want mozilla::Tie() to support both mozilla::Pair and std::pair, since both are used in our codebase.
mozilla::Pair and std::pair are supported by specialize a 2-tuple implementation.

@seth: do you think this is what you want?
Attachment #8637070 - Flags: feedback?(seth)
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 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!
Attachment #8637070 - Flags: feedback?(seth) → feedback+
Assignee: nobody → xeonchen
Remove variadic parameters on 2-arg specialization.
Use explicit constructor to pass static checking.
Fix TestTuple.c compilation error on some platforms.
Attachment #8637070 - Attachment is obsolete: true
Attachment #8638248 - Flags: review?(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.
Attachment #8638248 - Flags: review?(nfroyd)
Attachment #8638248 - Flags: review?(botond)
Attachment #8638248 - Flags: feedback+
Attachment #8638248 - Attachment is obsolete: true
Attachment #8638248 - Flags: review?(nfroyd)
Attachment #8638401 - Flags: review?(nfroyd)
(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 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.
Attachment #8638401 - Flags: review?(nfroyd) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7dee2347f114
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Blocks: 1192356
You need to log in before you can comment on or make changes to this bug.