1061144 - It is possible to add an integer to RefPtr

Status: NEW

(Core :: MFBT, defect)

Description

[JW Wang [:jwwang] [:jw_wang]]

Give int64_t mAudioStartTime and RefPtr<AudioSink> mAudioSink, I wrote something like below:

mAudioStartTime + mAudioSink ? mAudioSink->GetPosition() : 0;

My intention is:
mAudioStartTime + (mAudioSink ? mAudioSink->GetPosition() : 0);

However, it turns out to be:
(mAudioStartTime + mAudioSink) ? mAudioSink->GetPosition() : 0;


I am surprised I didn't get an error for |mAudioStartTime + mAudioSink|. It make me think again if implicit conversion to T* for RefPtr a convenient but dangerous idea.

Comment 1

[Jeff Walden [:Waldo]]

It probably is convenient but dangerous, yes.  I think it was this way because that's how nsRefPtr before it worked.  If it's not that much work to change over to require explicit .get(), that'd be a good thing to do.  But I'm not sure how much I could hope that.  We'd probably have to add a new type for RefPtr-as-argument if we wanted to support this, because currently we probably just assume we can pass RefPtr instances to T* arguments.  Not a bad thing to do, just a lot of work.

Comment 2

[Simon Giesecke [:sg] [he/him]]

(In reply to Jeff Walden [:Waldo] from comment #1)

It probably is convenient but dangerous, yes. I think it was this way
because that's how nsRefPtr before it worked. If it's not that much work to
change over to require explicit .get(), that'd be a good thing to do.

I think this should really be done. The implicit conversion to a raw pointer is pretty confusing, not only in this case.

But
I'm not sure how much I could hope that.

We'd probably have to add a new
type for RefPtr-as-argument if we wanted to support this, because currently
we probably just assume we can pass RefPtr instances to T* arguments.

What does this mean? Why is a special type needed? Assuming that the argument type stays T*, we can just pass refPtr.get(). But often, if the recipient actually requires its own RefPtr, the argument type can be changed to RefPtr<T>, and we can pass refPtr or std::move(refPtr). With respect to refcounting, this is never worse as with a T* argument, but possibly better, as we can spare modifying the refcount at all in the latter, move case.

Not a bad thing to do, just a lot of work.

Just adding .get() can be easily done with a clang-based rule. Changing argument types is probably harder.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #2)

(In reply to Jeff Walden [:Waldo] from comment #1)

We'd probably have to add a new
type for RefPtr-as-argument if we wanted to support this, because currently
we probably just assume we can pass RefPtr instances to T* arguments.

What does this mean? Why is a special type needed? Assuming that the argument type stays T*, we can just pass refPtr.get().

I imagine that the new argument type would preserve implicit coercion; requiring .get() would make things...rather noisy, I think.

We have historically discouraged passing smart pointers by value to avoid unnecessary refcounting. I don't have a good sense of how often callees take strong references to T* vs. just calling methods on the passed T*.

Comment 4

[Simon Giesecke [:sg] [he/him]]

(In reply to Nathan Froyd [:froydnj] from comment #3)

(In reply to Simon Giesecke [:sg] [he/him] from comment #2)

(In reply to Jeff Walden [:Waldo] from comment #1)

We'd probably have to add a new
type for RefPtr-as-argument if we wanted to support this, because currently
we probably just assume we can pass RefPtr instances to T* arguments.

What does this mean? Why is a special type needed? Assuming that the argument type stays T*, we can just pass refPtr.get().

I imagine that the new argument type would preserve implicit coercion; requiring .get() would make things...rather noisy, I think.

Hm, ok, that might be an option. By that, one would enforce that a RefPtr

We have historically discouraged passing smart pointers by value to avoid unnecessary refcounting.

I very much agree this should be avoided. As I said, in the case the callee acquires a strong reference, changing the argument type from T* to RefPtr<T> is a precondition for avoiding unnecessary refcounting. If the callee doesn't acquire a strong reference, its argument type should not be changed to RefPtr<T>.

I don't have a good sense of how often callees take strong references to T* vs. just calling methods on the passed T*.

At least for constructors, it's the former in most cases. As part of Bug 1609133, I can check how many cases of the former exist.