Make it compile error when instantiate a Variant with duplicate type

RESOLVED FIXED in Firefox 53

Status

()

Core
MFBT
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: JamesCheng, Assigned: JamesCheng)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

11 months ago
I found that in our mfbt Variant implementation, if we write

Variant<int, char, int> v(1);

The compiler did not warn anything. Actually our implementation did not support storing multiple same types value unlike std::variant(c++17) did.

So I think reminding developer of some error message will be fine and make it more clear to know there may be some misuse of this.
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8813523 - Flags: review?(nfroyd)
(Assignee)

Comment 2

11 months ago
Make sure all platforms can pass the build.
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=0da30e3949a05a108624acbbf0703585bea77414

Comment 3

11 months ago
mozreview-review
Comment on attachment 8813523 [details]
Bug 1319673 - Make it compile error when instantiate a Variant with duplicate type.

https://reviewboard.mozilla.org/r/94978/#review95388

Feel free to take the r+ if you want it -- I just happened to be looking to see what implementation technique was used, and in particular whether it was something more clever than this (maybe using compiler intrinsics) or was just stuck with the usual approach.

::: mfbt/Variant.h:42
(Diff revision 1)
> +    IsSame<First, Second>::value ||
> +    IsFirstMatchAny<First, Rest...>::value;
> +};
> +
> +template <typename...>
> +struct CheckUnique;

I'd rename this to TypesAreDistinct, and IsFirstMatchAny to FirstTypeIsInRest.  "Check unique" doesn't immediately conjure up this meaning, and "is first match any" is just hard to parse for me.
Attachment #8813523 - Flags: review+

Comment 4

11 months ago
mozreview-review
Comment on attachment 8813523 [details]
Bug 1319673 - Make it compile error when instantiate a Variant with duplicate type.

https://reviewboard.mozilla.org/r/94978/#review95390

::: mfbt/Variant.h:52
(Diff revision 1)
> +template<typename First, typename... Rest>
> +struct CheckUnique<First, Rest...>
> +{
> +  static constexpr bool value =
> +    CheckUnique<Rest...>::value &&
> +    !IsFirstMatchAny<First, Rest...>::value;

Oh, and maybe flip these two checks so that the "tail call" is in tail position.  (Setting aside that it's not really quite like that in reality, here.)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

11 months ago
Thank you Waldo. Addressed the issues but I don't know why review board landable status is still "r?".
Keywords: checkin-needed

Comment 7

11 months ago
(In reply to James Cheng[:JamesCheng] from comment #6)
> Thank you Waldo. Addressed the issues but I don't know why review board
> landable status is still "r?".

seems Jeff need to finish the review to be able to use autolander
Flags: needinfo?(jacheng)
Keywords: checkin-needed
(Assignee)

Comment 8

11 months ago
OK.

Hi Jeff, 

Could you please take a look to make the status to be landable?

Thank you.
Flags: needinfo?(jacheng) → needinfo?(jwalden+bmo)

Comment 9

11 months ago
mozreview-review
Comment on attachment 8813523 [details]
Bug 1319673 - Make it compile error when instantiate a Variant with duplicate type.

https://reviewboard.mozilla.org/r/94978/#review95592

Curse you, stupid Mozreview.
If that wasn't adequate to make this landable -- I don't really have any idea whether it was, I just did something that might maybe work -- I dunno what you should do.  Land it manually uphill both ways, like people did when I was your age, I guess.  ;-)
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 11

11 months ago
Thanks Jeff,

May need sheriff's help... I could do nothing but mark check-in-needed again.

Maybe giving up autolander and land to inbound instead.
Keywords: checkin-needed

Comment 12

11 months ago
mozreview-review
Comment on attachment 8813523 [details]
Bug 1319673 - Make it compile error when instantiate a Variant with duplicate type.

https://reviewboard.mozilla.org/r/94978/#review95636

r+ already given by waldo
Attachment #8813523 - Flags: review+

Comment 13

11 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fcdea0f0691b
Make it compile error when instantiate a Variant with duplicate type. r=Tomcat,Waldo
Keywords: checkin-needed

Comment 14

11 months ago
James, Jeff, fixed the problem and landed on autoland :)
Keywords: checkin-needed

Comment 15

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fcdea0f0691b
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox53: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.