698003 - Add operator bool to nsAutoRef

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 1

[Karl Tomlinson (:karlt)]

Comment on attachment 570277 [details] [diff] [review]
Patch v1

>+    operator RawRef() {
>+        return mRawRef;
>+    }

Doesn't "operator typename SimpleRef::RawRef() const" of nsAutoRef provide this functionality?

>+    operator RawRef*() {
>+        return &mRawRef;
>+    }

I see why you added this, but it scares me to have an implicit cast-to-pointer
mean address-of.  A call site may look like the reference is not going to be
modified, but this cast means that it can be.

It also means that mRawRef will not be cleared if it is already set and the callee overwrites it, thus bypassing the safety that can be assumed by letting nsAutoRef manage ownership.

I don't know the best solution here.  Something like getter_Transfers for
nsAutoPtr, perhaps, but that may be more effort than its worth.

In most code using GError, there is only one place where it needs to be freed,
so nsAutoRef may be unnecessary there.

GPtrArray is the other use case I see.
This is a little ugly but would work:

  GPtrArray* devices;
  get_devices_and_transfer_ownership_to_out_parameter(&devices);
  nsAutoRef<GPtrArray> autoReleaseDevices(devices)

Again it may not be necessary in your situation.

>+    operator bool() {
>+        return HaveResource();
>+    }
>+

BasicPlanarYCbCrImage::GetAsSurface() uses "if (mSurface)" for
"nsCountedRef<nsMainThreadSurfaceRef> mSurface", and
nsWebMReader::DecodeAudioData() use "if (!holder)" for
"nsAutoRef<NesteggPacketHolder> holder".

What is different in your situation?

Comment 2

[Mounir Lamouri (:mounir)]

(In reply to Karl Tomlinson (:karlt) from comment #1)
> Comment on attachment 570277 [details] [diff] [review] [diff] [details] [review]
> Patch v1
> 
> >+    operator RawRef() {
> >+        return mRawRef;
> >+    }
> 
> Doesn't "operator typename SimpleRef::RawRef() const" of nsAutoRef provide
> this functionality?

Indeed.

> >+    operator RawRef*() {
> >+        return &mRawRef;
> >+    }
> 
> I see why you added this, but it scares me to have an implicit
> cast-to-pointer
> mean address-of.  A call site may look like the reference is not going to be
> modified, but this cast means that it can be.

I tend to agree it is scary. I was expecting this new method to be refused actually.

> I don't know the best solution here.  Something like getter_Transfers for
> nsAutoPtr, perhaps, but that may be more effort than its worth.

I think we should open a follow-up for that.

> In most code using GError, there is only one place where it needs to be
> freed,
> so nsAutoRef may be unnecessary there.

Unnecessary, I don't know. Easily removable, yes.

> GPtrArray is the other use case I see.
> This is a little ugly but would work:
> 
>   GPtrArray* devices;
>   get_devices_and_transfer_ownership_to_out_parameter(&devices);
>   nsAutoRef<GPtrArray> autoReleaseDevices(devices)
> 
> Again it may not be necessary in your situation.

This one was more useful because it was transparently handling the hypothetical situation when an error was occurring but |devices| was set. I don't know if that is possible. I guess we can assume it is not.

> >+    operator bool() {
> >+        return HaveResource();
> >+    }
> >+
> 
> BasicPlanarYCbCrImage::GetAsSurface() uses "if (mSurface)" for
> "nsCountedRef<nsMainThreadSurfaceRef> mSurface", and
> nsWebMReader::DecodeAudioData() use "if (!holder)" for
> "nsAutoRef<NesteggPacketHolder> holder".
> 
> What is different in your situation?

I think |if (mSurface)| and |if (!holder)| are using |operator RawRef()| here, then the returned value is checked. It's working for pointers but HaveRessource() is using |Traits::Void()| so using |operator bool| might work in more situations.
Does that make sense?

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Karl, would you be ok with that?

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 571031 [details] [diff] [review]
Patch v2

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #2)
> I think |if (mSurface)| and |if (!holder)| are using |operator RawRef()|
> here, then the returned value is checked. It's working for pointers but
> HaveRessource() is using |Traits::Void()| so using |operator bool| might
> work in more situations.
> Does that make sense?

What you say makes sense, but I'm not sure that we want operator bool to "work" in more situations or what "work" means.

I'm usually in the EIBTI camp.
At this point I don't see much of a case for adding an operator bool method.

I'm also uncomfortable with what it might mean for arbitrary RawRef types.
The RawRef may have its own operator bool with a different meaning.

When RawRef is an integer this operator bool may well behave differently from the integer-to-bool conversion, which could lead to code that doesn't do what it looks like it is doing.

Comment 5

[Mounir Lamouri (:mounir)]

I guess you meant r- when you removed the review (otherwise, I could technically land this ;)).

Anyway, it doesn't work arguing more than that for this issue. I understand your points and I will just trust your judgment.

Comment 6

[Mounir Lamouri (:mounir)]

By "work" I meant "worth".