Closed Bug 975246 Opened 6 years ago Closed 4 years ago

nsRefPtr should support operator |nsRefPtr->*funcType|

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: airpingu, Assigned: JamesCheng)

References

Details

Attachments

(3 files, 7 obsolete files)

937 bytes, patch
JamesCheng
: review+
Details | Diff | Splinter Review
7.19 KB, patch
JamesCheng
: review+
gerald
: feedback+
Details | Diff | Splinter Review
1.60 KB, patch
JamesCheng
: review+
Details | Diff | Splinter Review
Otherwise, we have to use nsRefPtr.get()->*funcType, where the get() can be eliminated.
Summary: nsRefPtr should support operator |->*funcType| → nsRefPtr should support operator |nsRefPtr->*funcType|
We already have:
https://hg.mozilla.org/mozilla-central/file/7010ab83a06e/xpcom/base/nsAutoPtr.h#l1052
although it's #ifdef'd out for MSVC.  What platform were you testing on?

It's also possible that we need more const variants of it, given that C++ 2011 says that the built-in operator->* are the set ([over.built], clause 11):

  For every quintuple (C1, C2, T, CV1, CV2), where C2 is
  a class type, C1 is the same type as C2 or is a
  derived class of C2, T is an object type or a function
  type, and CV1 and CV2 are cv-qualifier-seqs, there
  exist candidate operator functions of the form
    CV12 T & operator->*(CV1 C1*, CV2 T C2::*);
  where CV12 is the union of CV1 and CV2.

although I don't really know the rules for interaction of CV-qualifiers and templates.

(On the other hand, our other smart pointer classes probably should have this!  nsRefPtr and nsAutoPtr are the only ones with an operator->*.  Unless, removing it fixes the problem, that is.)
Flags: needinfo?(gene.lian)
Yeah, you won't be able to call this on a const nsRefPtr, so we should probably add a const overload.  Also, we might be able to turn it on for MSVC too, I think back when you added this in bug 525205 we were not using MSVC 2010 yet.
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #1)
> We already have:
> https://hg.mozilla.org/mozilla-central/file/7010ab83a06e/xpcom/base/
> nsAutoPtr.h#l1052
> although it's #ifdef'd out for MSVC.  What platform were you testing on?

Linux build and I'm sure I have to add an extra .get() to comiple it. Yeap, addig a const variant might solve this issue.

> 
> It's also possible that we need more const variants of it, given that C++
> 2011 says that the built-in operator->* are the set ([over.built], clause
> 11):
> 
>   For every quintuple (C1, C2, T, CV1, CV2), where C2 is
>   a class type, C1 is the same type as C2 or is a
>   derived class of C2, T is an object type or a function
>   type, and CV1 and CV2 are cv-qualifier-seqs, there
>   exist candidate operator functions of the form
>     CV12 T & operator->*(CV1 C1*, CV2 T C2::*);
>   where CV12 is the union of CV1 and CV2.
> 
> although I don't really know the rules for interaction of CV-qualifiers and
> templates.

Thanks David for sharing this info! Interesting!
Flags: needinfo?(gene.lian)
Hi Nathan,

I just saw the code 
https://dxr.mozilla.org/mozilla-central/source/dom/promise/Promise.cpp?from=promise.cpp#1516

which mentioned this bug.

I attach this patch for supporting the operator->* in nsRefPtr.

Could you please help me for reviewing the patches or any recommend reviewer?

Here is the test result for reference.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=88a20b9987e3

Thank you very much.
Attachment #8638035 - Flags: review?(nfroyd)
Assignee: nobody → jacheng
Update the test case to invoke the member function via operator->*
Attachment #8638036 - Flags: review?(nfroyd)
Do not use nsRefPtr.get()->* and use nsRefPtr->* directly instead.

Just want to make sure it is instantiated and can be compiled.
Attachment #8638039 - Flags: feedback?(nfroyd)
Comment on attachment 8638035 [details] [diff] [review]
Part1-Implement-operator-in-nsRefPtr.patch

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

This seems reasonable.  I think it removes the ability to use pointer-to-member variables with nsRefPtr, though judging by the try run, that doesn't look like a problem.

Gerald volunteered to be a second pair of eyes on tricky C++ things, so I'm going to have him take a second look.  Gerald, is simply adding more const-qualified variants of the existing operator->* all that's necessary here, or do we need to go with the proxy approach?  I don't see operator->* implemented on the STL smart pointer classes, so I'm wondering if that's an oversight or a deliberate decision, too...
Attachment #8638035 - Flags: review?(nfroyd)
Attachment #8638035 - Flags: review?(gsquelart)
Attachment #8638035 - Flags: review+
Comment on attachment 8638036 [details] [diff] [review]
Part2-Test-Invoking-via-operator.patch

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

Thanks for adding tests!
Attachment #8638036 - Flags: review?(nfroyd) → review+
Comment on attachment 8638039 [details] [diff] [review]
Part3-try-to-instantiate-the-template-method.patch

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

::: dom/promise/Promise.cpp
@@ +1517,1 @@
>                              value);

Removing the TODO comment and putting this all on one line would be good.
Attachment #8638039 - Flags: feedback?(nfroyd) → feedback+
Hi Nathan,

Thanks for your reviewing,

I update this patch and please help to check again.

Thank you very much.

Hi Gerald,

Thank you for helping.
Attachment #8638039 - Attachment is obsolete: true
Attachment #8639668 - Flags: review?(nfroyd)
Carry r+ from nfroyd.
Attachment #8638036 - Attachment is obsolete: true
Attachment #8639669 - Flags: review+
Comment on attachment 8639668 [details] [diff] [review]
Part3-Using-nsRefPtr-operator.-r-nfroyd.patch

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

Please change the commit message to "Bug 975246 - Part 3 - use nsRefPtr::operator->* in Promise.cpp; r=nfroyd".  Thanks!
Attachment #8639668 - Flags: review?(nfroyd) → review+
Comment on attachment 8638035 [details] [diff] [review]
Part1-Implement-operator-in-nsRefPtr.patch

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

I admit I didn't know about this operator, it's actually quite ancient!
A bit of research led me to http://www.aristeia.com/Papers/DDJ_Oct_1999.pdf , which also adopts a proxy style (and like Nathan noticed, it mentions that you can’t use pointers to data members).

In any case it does the job for where it's intended to be used, so I'm fine with it too; We can always revisit if someone needs to use it with a pointer to data member.
I'd like to see more tests in the 2nd patch, to ensure that const/virtual methods work.
Attachment #8638035 - Flags: review?(gsquelart) → review+
Hi Gerald,

Thanks for reviewing.

I will modified the 2nd patch test code with 
1. const nsRefPtr->*
2. invoke virtual function with polymorphism.

and attach the test result for you.

Thanks.
Carry r+ from nfroyd and gerald. update the patch description.
Attachment #8638035 - Attachment is obsolete: true
Attachment #8640313 - Flags: review+
Carry r+ from nfroyd. Update the patch description.
Attachment #8639668 - Attachment is obsolete: true
Attachment #8640314 - Flags: review+
Carry r+ and update the description.

Hi Gerald,

I update the patch and tested the virtual with polymorphism cases.

nsRefPtr will try to invoke |AddRef()| which is without const 

so basically we cannot use |nsRefPtr<const XXX>| in gecko.

Please help to have some feedback of this patch.

Thank you very much~
Attachment #8639669 - Attachment is obsolete: true
Attachment #8640318 - Flags: review+
Attachment #8640318 - Flags: feedback?(gsquelart)
Comment on attachment 8640318 [details] [diff] [review]
Part2-Test-Invoking-via-operator-.-r-nfro.patch

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

(In reply to James Cheng[:JamesCheng] from comment #17)
> Created attachment 8640318 [details] [diff] [review]
> Part2-Test-Invoking-via-operator-.-r-nfro.patch
> 
> I update the patch and tested the virtual with polymorphism cases.

Thanks, looks good. Did the output of the test show that the virtual calls foop2->*fV[C]Ptr correctly went to the Bar overrides? (No time to test it myself right now, sorry.)

> nsRefPtr will try to invoke |AddRef()| which is without const 
> so basically we cannot use |nsRefPtr<const XXX>| in gecko.

I have found instances of classes with const AddRef&Release (and the corresponding thread-safe mutable ref counter), e.g. OverscrollHandoffState in gfx/layers/apz/src -- though it looks like a unique case involving 3rd party software.

Ideally we should test all combinations based on the C++ specs (see comment 1), or pragmatically at least those that do appear in our code.
Attachment #8640318 - Flags: feedback?(gsquelart) → feedback+
Hi Gerald,

I've also found this special case for nsRefPtr<const T> using mutable refcount.

For comprehensive testing, I update the test case to test nsRefPtr<const T>.

For the question "Did the output of the test show that the virtual calls foop2->*fV[C]Ptr correctly went to the Bar overrides?"

Yes, it works fine.

Please help to take a look of this patch.

Thank you.
Attachment #8640318 - Attachment is obsolete: true
Attachment #8640405 - Flags: review+
Attachment #8640405 - Flags: feedback?(gsquelart)
Comment on attachment 8640405 [details] [diff] [review]
Part2-Test-Invoking-via-operator-.-r-nfro.patch

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

Thank you for the update, looks good to me.
Attachment #8640405 - Flags: feedback?(gsquelart) → feedback+
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
Rebase for the patch.
Attachment #8640313 - Attachment is obsolete: true
Attachment #8640861 - Flags: review+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.