Closed Bug 1367674 Opened 3 years ago Closed 3 years ago

TakesArgumentHelper should take CV into account

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

Details

Attachments

(2 files)

http://searchfox.org/mozilla-central/rev/35b37316149108d53a02fb8498e250ea8a22ab5d/xpcom/threads/MozPromise.h#42

Though we rarely have volatile member functions.

Also it doesn't work for a method that have more than 2 arguments.
Attachment #8872143 - Flags: review?(gsquelart)
Attachment #8872144 - Flags: review?(gsquelart)
Assignee: nobody → jwwang
Comment on attachment 8872143 [details]
Bug 1367674. P1 - add templates to deal with CV and argument number correctly.

https://reviewboard.mozilla.org/r/143624/#review147362

r+ with style nit, if you agree:

::: xpcom/threads/MozPromise.h:101
(Diff revision 1)
> -struct TakesArgument {
> -  static const bool value = decltype(detail::TakesArgumentHelper(DeclVal<MethodType>()))::value;
> +using TakesArgument =
> +  IntegralConstant<bool, !!detail::MethodTrait<MethodType>::ArgSize>;

Instead of `!!ArgSize`, I'd prefer `ArgSize != 0`.
(it's clearer to me as we're dealing with a number; only use `!` with bools and pointers)
Attachment #8872143 - Flags: review?(gsquelart) → review+
Comment on attachment 8872144 [details]
Bug 1367674. P2 - remove unused code.

https://reviewboard.mozilla.org/r/143626/#review147364
Attachment #8872144 - Flags: review?(gsquelart) → review+
Thanks!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6dea98f4fbc2
P1 - add templates to deal with CV and argument number correctly. r=gerald
https://hg.mozilla.org/integration/autoland/rev/02d43f8e8686
P2 - remove unused code. r=gerald
https://hg.mozilla.org/mozilla-central/rev/6dea98f4fbc2
https://hg.mozilla.org/mozilla-central/rev/02d43f8e8686
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.