Closed Bug 1319673 Opened 3 years ago Closed 3 years ago

Make it compile error when instantiate a Variant with duplicate type

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

Details

Attachments

(1 file)

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.
Attachment #8813523 - Flags: review?(nfroyd)
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 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.)
Thank you Waldo. Addressed the issues but I don't know why review board landable status is still "r?".
Keywords: checkin-needed
(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
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 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)
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 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+
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
James, Jeff, fixed the problem and landed on autoland :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fcdea0f0691b
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.