1189231 - nsAutoPtr/nsRefPtr should support operator |ptr->*funcType|

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[James Cheng[:JamesCheng]]

Since Bug 975246 is fixed, we should align the implementation.

So I just attach a patch to fix it in nsAutoPtr.

https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsAutoPtr.h?from=nsautoptr.h#206

Comment 1

[James Cheng[:JamesCheng]]

Attachment: Impl-operator->* to-nsAutoPtr.

Hello Nathan,

The implementation is quite similar with Bug 975246. 

I also modified the test case.

Please help me for reviewing the bug.

Thank you very much.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8641002 [details] [diff] [review]
Impl-operator->* to-nsAutoPtr.

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

::: xpcom/base/nsAutoPtr.h
@@ +210,5 @@
> +    {
> +    }
> +    R operator()(Args... aArgs)
> +    {
> +      return ((*mRawPtr).*mFunction)(mozilla::Forward<Args>(aArgs)...);

I confess that I'm a little surprised to see mozilla::Forward being used without rvalue reference args.  I guess you can do this, but I'm not sure it does what you expect.

Does this construct do the right thing and not copy arguments unexpectedly and/or do the right thing when it comes to mozilla::Move'd arguments?  Tests to prove that the correct behavior happens would be ideal.

I realize that the nsRefPtr version probably has this same issue, and I apologize for not catching that in review earlier.

@@ +220,3 @@
>    {
>      NS_PRECONDITION(mRawPtr != 0,
> +                    "You can't dereference a NULL nsRefPtr with operator->*().");

This error message should still refer to nsAutoPtr.

Comment 3

[James Cheng[:JamesCheng]]

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #2)
> Comment on attachment 8641002 [details] [diff] [review]
> Impl-operator->* to-nsAutoPtr.
> 
> Review of attachment 8641002 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/base/nsAutoPtr.h
> @@ +210,5 @@
> > +    {
> > +    }
> > +    R operator()(Args... aArgs)
> > +    {
> > +      return ((*mRawPtr).*mFunction)(mozilla::Forward<Args>(aArgs)...);
> 
> I confess that I'm a little surprised to see mozilla::Forward being used
> without rvalue reference args.  I guess you can do this, but I'm not sure it
> does what you expect.
> 
Hi Nathan,
Yes, I know Forward is used to use in perfect forwarding condition.
typically in
class Clazz{};
template<class T>
void Bar(T&& arg)
{
  Foo(std::forward(arg));
}
When invoking Bar(Clazz) with l value, the T will be deduced to Clazz& and T&& will be Clazz&&& which is folded into Clazz&.
When invoking Bar(std::move(Clazz)) with R value, the T will be deduced to Clazz, and T&& will be Clazz&&.
and the forwarding semantic is to "keep the original type" and forward to another function.
https://dxr.mozilla.org/mozilla-central/source/mfbt/Move.h#211

In my case for operator->*,
It is intended to do this.
Let me explain for the purpose. 

Case 1: Type Args is Clazz(call by value)
The Clazz will be copied into functor(operator()), the semantic is just like a |call by value| function call,
Then we use Forward semantic to let the arg be an R value type.(it is not the same with perfect forwarding case, just do a casting). then passing this to the real function call will avoid "copying again" occurred.

Case 2: Type Args is Clazz&(call by ref), the Forward statement returns Clazz&&& which is folded into Clazz&, and pass to the real member funciton call(no copy occurred and keep the same semantic as normal function call).

Case 3: Type Args is Clazz&&(call by r value ref), the Forward statement keeps the same type back as Clazz&&&& which can be folded into Clazz&&. passing to the real member function call(no copy occurred and keep the same semantic as normal function call).
 

But thanks for your reminding,
I can refine this patch with more elegant way as below,

    template<typename... ActualArgs>
    R operator()(ActualArgs&&... aArgs)
    {
      return ((*mRawPtr).*mFunction)(mozilla::Forward<ActualArgs>(aArgs)...);
    }

Use template argument deduction and perfect forwarding to make Case 1 prevent from a |move| action.
(pass reference to operator() and forward to real function call with a copy).

Please help me for reviewing the new patch.

Thank you very much.


> Does this construct do the right thing and not copy arguments unexpectedly
> and/or do the right thing when it comes to mozilla::Move'd arguments?  Tests
> to prove that the correct behavior happens would be ideal.
> 
> I realize that the nsRefPtr version probably has this same issue, and I
> apologize for not catching that in review earlier.



> 
> @@ +220,3 @@
> >    {
> >      NS_PRECONDITION(mRawPtr != 0,
> > +                    "You can't dereference a NULL nsRefPtr with operator->*().");
> 
> This error message should still refer to nsAutoPtr.

Addressed, thanks~

Comment 4

[James Cheng[:JamesCheng]]

Attachment: Impl-operator-to-nsAutoPtr.-r-nfroyd.patch

Hi Nathan,
Please help to review the revised patch according to the comment 3

Thanks.

Comment 5

[James Cheng[:JamesCheng]]

Attachment: Refine-operator-call-with-perfect-forwarding in nsRefPtr

Hi Nathan,

Thanks for reminding that I also modified the nsRefPtr version.

Please also take a look at this patch.

Thanks.

Comment 6

[James Cheng[:JamesCheng]]

Thanks for your reviewing.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please provide a Try link.

Comment 8

[James Cheng[:JamesCheng]]

Sorry for forgetting to provide a try link.

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

Thank you.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/312dce5d5216
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cadf71b19b2

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/312dce5d5216
https://hg.mozilla.org/mozilla-central/rev/5cadf71b19b2