1212745 - Make mozilla::Function support pointer to member function

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[James Cheng[:JamesCheng]]

Currently, the mozilla::Function did not support passing member function pointer as a callable object.

The usage of std::function with member function is showing on below

http://ideone.com/xCYccE

#include <iostream>
#include <functional>
using namespace std;
class Foo
{
public:
	void A(int) {cout<<"A"<<endl;}
	void B(int) const {cout<<"B"<<endl;}
};
int main() {
	Foo foo;
	std::function<void(Foo&, int)> f1 = &Foo::A;
	std::function<void(Foo*, int)> f2 = &Foo::B;
	f1(foo, 1);
	f2(&foo, 2);
	return 0;
}

For providing a comprehensive mozilla::Function, I am writing a patch for this.

Comment 1

[James Cheng[:JamesCheng]]

Attachment: Make-mozilla-Function-support-pointer-to.patch

Hi Botond,
Could you please take a look at this patch?

I also add the test case.

If it is OK for reviewing, please recommend me a suitable reviewer for this patch. 

Thank you very much.

Comment 2

[Botond Ballo [:botond]]

Comment on attachment 8671205 [details] [diff] [review]
Make-mozilla-Function-support-pointer-to.patch

Review of attachment 8671205 [details] [diff] [review]:
-----------------------------------------------------------------

This approach looks good to me. Thanks for working on it!

:froydnj reviewed the original patch adding mozilla::Function; he would be a good reviewer for this.

You may want to consider splitting the patch into a first part which just moves FunctionImplBase and FunctionImpl inside Function (you can probably just call them ImplBase and Impl now that they're inside Function), and a second part which adds the new base class and specializations.

::: mfbt/Function.h
@@ +148,4 @@
>      return *this;
>    }
>  
> +  template<typename... Args>

Why is it necessary to make the call operator a template? This seems to me like a separate change which - if we're going to make it at all - should be reviewed separately.

Comment 3

[James Cheng[:JamesCheng]]

Attachment: Part1-Make-mozilla-Function-support-pointer-to.patch

(In reply to Botond Ballo [:botond] from comment #2)
> Comment on attachment 8671205 [details] [diff] [review]
> Make-mozilla-Function-support-pointer-to.patch
> 
> Review of attachment 8671205 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This approach looks good to me. Thanks for working on it!
> 
> :froydnj reviewed the original patch adding mozilla::Function; he would be a
> good reviewer for this.
> 
> You may want to consider splitting the patch into a first part which just
> moves FunctionImplBase and FunctionImpl inside Function (you can probably
> just call them ImplBase and Impl now that they're inside Function), and a
> second part which adds the new base class and specializations.
> 
I encountered some inevitable compiler internal issue on VS2013....VS2015 works fine. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=429e4201794b

Finally, I found if I don't merge the class inside class Function, it works fine.

So I will submit a new patch which is separate the impl class in mozilla::detail.

> ::: mfbt/Function.h
> @@ +148,4 @@
> >      return *this;
> >    }
> >  
> > +  template<typename... Args>
> 
> Why is it necessary to make the call operator a template? This seems to me
> like a separate change which - if we're going to make it at all - should be
> reviewed separately.

Since this operator() just forward the parameter to the real function.

If you don't use template with perfect forwarding, it will make a redundant copy.

So I change it to the perfect forwarding pattern.

Thanks for your feedback. Carry f+ from Botond.

Hi Nathan,

Could you please review this patch for mozilla::Function supporting pointer to member function?

Thanks.

Comment 4

[James Cheng[:JamesCheng]]

Attachment: Part2-operator-Use-perfect-forwarding-to-avoid.patch

Comment 5

[James Cheng[:JamesCheng]]

Attachment: Part1-Make-mozilla-Function-support-pointer-to.patch

update for some typo.

Comment 6

[James Cheng[:JamesCheng]]

Attach the treeherder result for reference.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cd530722460

Comment 7

[Sebastian Hengst [:aryx] [limited availability until start of 2020] (needinfo on intermittent or backout)]

Part1 of the patches fails to apply, hg doesn't provide more details. Please check the patch for issues. Thank you.

Comment 8

[James Cheng[:JamesCheng]]

Attachment: Part1-Make-mozilla-Function-support-pointer-to.patch

Hi Sebastian,

Fixed this patch, thank you.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d63e439e42
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6261091eca7

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a5d63e439e42
https://hg.mozilla.org/mozilla-central/rev/f6261091eca7