Make mozilla::Function support pointer to member function

RESOLVED FIXED in Firefox 44

Status

()

Core
MFBT
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: JamesCheng, Assigned: JamesCheng)

Tracking

unspecified
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8671205 [details] [diff] [review]
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.
Attachment #8671205 - Flags: feedback?(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.
Attachment #8671205 - Flags: feedback?(botond) → feedback+

Updated

2 years ago
Depends on: 1198451
(Assignee)

Comment 3

2 years ago
Created attachment 8671524 [details] [diff] [review]
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.
Attachment #8671205 - Attachment is obsolete: true
Attachment #8671524 - Flags: review?(nfroyd)
Attachment #8671524 - Flags: feedback+
(Assignee)

Comment 4

2 years ago
Created attachment 8671526 [details] [diff] [review]
Part2-operator-Use-perfect-forwarding-to-avoid.patch
Attachment #8671526 - Flags: review?(nfroyd)
(Assignee)

Comment 5

2 years ago
Created attachment 8671535 [details] [diff] [review]
Part1-Make-mozilla-Function-support-pointer-to.patch

update for some typo.
Attachment #8671524 - Attachment is obsolete: true
Attachment #8671524 - Flags: review?(nfroyd)
Attachment #8671535 - Flags: review?(nfroyd)
Attachment #8671535 - Flags: feedback+
(Assignee)

Comment 6

2 years ago
Attach the treeherder result for reference.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cd530722460
Attachment #8671535 - Flags: review?(nfroyd) → review+
Attachment #8671526 - Flags: review?(nfroyd) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Part1 of the patches fails to apply, hg doesn't provide more details. Please check the patch for issues. Thank you.
Flags: needinfo?(jacheng)
(Assignee)

Comment 8

2 years ago
Created attachment 8672439 [details] [diff] [review]
Part1-Make-mozilla-Function-support-pointer-to.patch

Hi Sebastian,

Fixed this patch, thank you.
Attachment #8671535 - Attachment is obsolete: true
Flags: needinfo?(jacheng)
Attachment #8672439 - Flags: review+

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d63e439e42
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6261091eca7
Keywords: checkin-needed

Comment 10

2 years ago
bugherdermerge
https://hg.mozilla.org/mozilla-central/rev/a5d63e439e42
https://hg.mozilla.org/mozilla-central/rev/f6261091eca7
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.