Closed Bug 1371880 Opened 2 years ago Closed 2 years ago

Use Variant with indices to simplify MozPromise::ResolveOrRejectValue

Categories

(Core :: XPCOM, enhancement)

55 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

Details

Attachments

(1 file)

Bug 1363676 made ResolveOrRejectValue use Variant (instead of two Maybe's).
But because Variant didn't allow duplicate types then, JW used a `Holder<int, T>` struct to distinguish between resolve and reject types when the underlying types could actually actually be the same!

I think we could now simplify this and remove the Holder pattern, by using Variant's new indexed access, which allows same types (see bug 1338389).
Comment on attachment 8876539 [details]
Bug 1371880 - Use Variant with indexed access in MozPromise::ResolveOrRejectValue -

https://reviewboard.mozilla.org/r/147866/#review152216

::: xpcom/threads/MozPromise.h:233
(Diff revision 1)
>      {
> -      return mValue.template as<RejectValueHolder>().mData;
> +      return mValue.template as<RejectIndex>();
>      }
>  
>    private:
> -    Variant<Nothing, ResolveValueHolder, RejectValueHolder> mValue = AsVariant(Nothing{});
> +    enum { NothingIndex, ResolveIndex, RejectIndex };

Does the C++ spec. guarantee NothingIndex to be 0?
Attachment #8876539 - Flags: review?(jwwang) → review+
Comment on attachment 8876539 [details]
Bug 1371880 - Use Variant with indexed access in MozPromise::ResolveOrRejectValue -

https://reviewboard.mozilla.org/r/147866/#review152216

Thank you for the quick review.

> Does the C++ spec. guarantee NothingIndex to be 0?

Yes, in [dcl.enum]: "If the first enumerator has no initializer, the value of the corresponding constant is zero."
(But thank you for asking, better be sure!)
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fab1fc13a649
Use Variant with indexed access in MozPromise::ResolveOrRejectValue - r=jwwang
https://hg.mozilla.org/mozilla-central/rev/fab1fc13a649
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.