Provide an overloaded == operator for mozilla::Variant

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla42
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
Created attachment 8635111 [details] [diff] [review]
Provide an overloaded == operator for mozilla::Variant
Attachment #8635111 - Flags: review?(nfroyd)
Comment on attachment 8635111 [details] [diff] [review]
Provide an overloaded == operator for mozilla::Variant

Add an operator!= returning the negation of equality, please.

And I guess there's no need yet for T == Variant<..., T, ...> or vice versa yet, right?  So no need to try to futz that into place.  Seems like if comparing two variants is desirable, where the stored type of each is unknown, the more-precise comparison where a variant is compared against (in effect) a variant containing a *known* type is a fortiori desirable.
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Comment on attachment 8635111 [details] [diff] [review]
> Provide an overloaded == operator for mozilla::Variant
> 
> Add an operator!= returning the negation of equality, please.
> 
> And I guess there's no need yet for T == Variant<..., T, ...> or vice versa
> yet, right?  So no need to try to futz that into place.  Seems like if
> comparing two variants is desirable, where the stored type of each is
> unknown, the more-precise comparison where a variant is compared against (in
> effect) a variant containing a *known* type is a fortiori desirable.

Yeah that certainly seems like a good thing to support, but yeah my approach so far has been to add things only as I need them. Filed bug 1185050 for this.
Created attachment 8635481 [details] [diff] [review]
Provide an overloaded == operator for mozilla::Variant
Attachment #8635111 - Attachment is obsolete: true
Attachment #8635111 - Flags: review?(nfroyd)
Attachment #8635481 - Flags: review?(nfroyd)
Comment on attachment 8635481 [details] [diff] [review]
Provide an overloaded == operator for mozilla::Variant

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

::: mfbt/tests/TestVariant.cpp
@@ +109,5 @@
> +  MOZ_RELEASE_ASSERT(v0 != v1);
> +  MOZ_RELEASE_ASSERT(v1 == v2);
> +  MOZ_RELEASE_ASSERT(v2 != v3);
> +  MOZ_RELEASE_ASSERT(v3 != v4);
> +  MOZ_RELEASE_ASSERT(v4 == v5);

Also seems worth having

  V v6(int('b'));

and perhaps others, comparing that against V('b') and verifying they're not equal.  You've tested the different-tags-different-values case, the same-tags-but-different-values case, and the same-tags-values cases.  That leaves the different-tags-same-values case worth testing.
Attachment #8635481 - Flags: review?(nfroyd) → review+
Created attachment 8635840 [details] [diff] [review]
Provide an overloaded == operator for mozilla::Variant
Attachment #8635481 - Attachment is obsolete: true
Attachment #8635840 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/53d4da7e97f1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.