1319673 - Make it compile error when instantiate a Variant with duplicate type

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[James Cheng[:JamesCheng]]

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 1

[James Cheng[:JamesCheng]]

Attachment: Bug 1319673 - Make it compile error when instantiate a Variant with duplicate type.

Review commit: https://reviewboard.mozilla.org/r/94978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94978/

Comment 2

[James Cheng[:JamesCheng]]

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

Comment 3

[Jeff Walden [:Waldo]]

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.

Comment 4

[Jeff Walden [:Waldo]]

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 5

[James Cheng[:JamesCheng]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94978/diff/1-2/

Comment 6

[James Cheng[:JamesCheng]]

Thank you Waldo. Addressed the issues but I don't know why review board landable status is still "r?".

Comment 7

[Carsten Book [:Tomcat]]

(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

Comment 8

[James Cheng[:JamesCheng]]

OK.

Hi Jeff, 

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

Thank you.

Comment 9

[Jeff Walden [:Waldo]]

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.

Comment 10

[Jeff Walden [:Waldo]]

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.  ;-)

Comment 11

[James Cheng[:JamesCheng]]

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.

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fcdea0f0691b
Make it compile error when instantiate a Variant with duplicate type. r=Tomcat,Waldo

Comment 14

[Carsten Book [:Tomcat]]

James, Jeff, fixed the problem and landed on autoland :)

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fcdea0f0691b