Closed Bug 1232418 Opened 4 years ago Closed 4 years ago

Allow equality comparison of Tuple types

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- affected
firefox48 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(2 files)

As discussed on IRC before the work week.
Attachment #8698146 - Flags: review?(jwalden+bmo)
Comment on attachment 8698146 [details] [diff] [review]
teach_Tuple_operatorEq-v0.diff

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

I'm a bit confused why Head/Tail are publicly exposed, but that's aside from this bug/patch.  (And it looks like there might be other things publicly expose that shouldn't be as well.  Still a separate bug.)

::: mfbt/Tuple.h
@@ +186,5 @@
>      Head(*this) = Move(Head(aOther));
>      Tail(*this) = Move(Tail(aOther));
>      return *this;
>    }
> +  bool operator==(const TupleImpl& aOther) const

Can this method and the empty-case operator== above be protected instead of public?  Tuple::operator== is the only one that we really want exposed.
Attachment #8698146 - Flags: review?(jwalden+bmo) → review+
Attached file build_output.txt
It seems that no, we cannot make those protected. The failing build output is inscrutable in the way that only C++ template errors can be inscrutable, but what I think it comes down to is: for TupleImpl<n, HeadT, TailT...>, we are derived from TupleImpl<n+1, TailT...>, however, Head and Tail return HeadT and TailT refs which are not the same as Base. They are also TupleImpls, but are not on our ancestor chain, so we don't have protected access there. We might be able to friend ourself to HeadT and TailT? I'm not very confident in C++'s friendship model, and I've rarely had luck with template friends.
https://hg.mozilla.org/mozilla-central/rev/b668ab1846b0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.