1068198 - Allow assigning RefPtr to TemporaryRef of child class.

Status: RESOLVED INVALID

(Core :: MFBT, defect)

Description

[Jacek Caban]

Attachment: patch.diff

This will allow code like the one causing problems in bug 1068193 on non-MSVC compilers (MSVC is less strict about casts and already allows it).

Comment 1

[Jacek Caban]

Here is try push:

https://tbpl.mozilla.org/?tree=Try&rev=48efb73d27bf

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8490261 [details] [diff] [review]
patch.diff

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

Sorry for the delay here.  :-(  While this seems fine if we actually want to allow this, I'm not entirely sure we do.  (I was *this* close to r+ until I thought a little harder.)  Assigning a RefPtr<Derived> into a RefPtr<Base> is fine in theory, but in practice -- because the methods you can call through a RefPtr aren't just the virtual ones, or the ones that are guaranteed to be the same in Base as in all Derived -- a method call or whatever could call the base-class thing, when it's important that the derived-class version be used.

If the original rationale for this was an exceptional case, and most assignments are of T to T and not U to T, I think we should do sort of the opposite -- forbid assignments of U to T.  Could you instead add deleted versions of the new template overloads in your patch, and see how that fares?  Better to be cautious and allow little than optimistic and silently shoot ourselves in the foot later, seems to me.

Comment 3

[Jeff Walden [:Waldo]]

(Put the deleted functions into a private: section in the class, too, please.  MOZ_DELETE doesn't yet work everywhere, so putting them in a private section means a use of them by a non-friend is an immediate compile error, instead of just an undefined-function link error if the "deleted" function were public.)

Comment 4

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

Bug 1161627 removed TemporaryRef, so this seems obsolete. Please reopen with an updated description if it is still relevant.