975246 - nsRefPtr should support operator |nsRefPtr->*funcType|

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Gene Lian (I already quit Mozilla)]

Otherwise, we have to use nsRefPtr.get()->*funcType, where the get() can be eliminated.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 2

[(no longer active)]

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.

Comment 3

[Gene Lian (I already quit Mozilla)]

(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!

Comment 4

[James Cheng[:JamesCheng]]

Attachment: Part1-Implement-operator-in-nsRefPtr.patch

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.

Comment 5

[James Cheng[:JamesCheng]]

Attachment: Part2-Test-Invoking-via-operator.patch

Update the test case to invoke the member function via operator->*

Comment 6

[James Cheng[:JamesCheng]]

Attachment: Part3-try-to-instantiate-the-template-method.patch

Do not use nsRefPtr.get()->* and use nsRefPtr->* directly instead.

Just want to make sure it is instantiated and can be compiled.

Comment 7

[Nathan Froyd [:froydnj]]

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...

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8638036 [details] [diff] [review]
Part2-Test-Invoking-via-operator.patch

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

Thanks for adding tests!

Comment 9

[Nathan Froyd [:froydnj]]

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.

Comment 10

[James Cheng[:JamesCheng]]

Attachment: Part3-Using-nsRefPtr-operator.-r-nfroyd.patch

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.

Comment 11

[James Cheng[:JamesCheng]]

Attachment: Part2-Test-Invoking-via-operator-.-r-nfro.patch

Carry r+ from nfroyd.

Comment 12

[Nathan Froyd [:froydnj]]

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!

Comment 13

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 14

[James Cheng[:JamesCheng]]

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.

Comment 15

[James Cheng[:JamesCheng]]

Attachment: Part1-Implement-operator-in-nsRefPtr.-r-n.patch

Carry r+ from nfroyd and gerald. update the patch description.

Comment 16

[James Cheng[:JamesCheng]]

Attachment: Part3-Using-nsRefPtr-operator.-r-nfroyd.patch

Carry r+ from nfroyd. Update the patch description.

Comment 17

[James Cheng[:JamesCheng]]

Attachment: Part2-Test-Invoking-via-operator-.-r-nfro.patch

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~

Comment 18

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 19

[James Cheng[:JamesCheng]]

Attachment: Part2-Test-Invoking-via-operator-.-r-nfro.patch

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.

Comment 20

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 22

[James Cheng[:JamesCheng]]

Attachment: Part1-Implement-operator-in-nsRefPtr.-r-n.patch

Rebase for the patch.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9d25e716d31
https://hg.mozilla.org/integration/mozilla-inbound/rev/c016da5a123d
https://hg.mozilla.org/integration/mozilla-inbound/rev/407507029695

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c9d25e716d31
https://hg.mozilla.org/mozilla-central/rev/c016da5a123d
https://hg.mozilla.org/mozilla-central/rev/407507029695