nsRefPtr should be able to forget a base class to a pure virtual interface it inherits from

RESOLVED FIXED in mozilla1.9.2a1

Status

()

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 years ago
say you have an outparm, nsISupports **_retval, and you have some base object that inherits from nsISupports, nsFoo.  Right now, you can't do this:

nsRefPtr<nsFoo> foo;
foo.forget(_retval);
(Assignee)

Updated

10 years ago
Blocks: 489702
(Assignee)

Comment 1

10 years ago
Created attachment 374185 [details] [diff] [review]
v1.0

This does the trick for me.  I'm pushing this to the try server now.
Attachment #374185 - Flags: superreview?(benjamin)
Attachment #374185 - Flags: review?(dbaron)
(Assignee)

Updated

10 years ago
Whiteboard: [needs review dbaron][needs sr bsmedberg]
Seems like you should probably get rid of the forget(T**) since this should cover it.  (Also, if you're going to ask both benjamin and me for reviews, probably better to swap r/sr.)
And a random thought, in case anybody ever sees it here:

If we ever do the same thing for nsCOMPtr (which might be sensible given that interfaces can inherit from other interfaces), we should have the equivalent of an NSCAP_ASSERT_NO_QUERY_NEEDED on the result.
(Assignee)

Comment 4

10 years ago
Created attachment 374189 [details] [diff] [review]
v1.1

(In reply to comment #2)
> Seems like you should probably get rid of the forget(T**) since this should
> cover it.  (Also, if you're going to ask both benjamin and me for reviews,
> probably better to swap r/sr.)
I didn't want to change too much, even though I was pretty sure it'd be alright to do that.  My test compile is doing just fine, and the diff is simpler now :)
Attachment #374185 - Attachment is obsolete: true
Attachment #374189 - Flags: superreview?(dbaron)
Attachment #374189 - Flags: review?(benjamin)
Attachment #374185 - Flags: superreview?(benjamin)
Attachment #374185 - Flags: review?(dbaron)
(Assignee)

Updated

10 years ago
Whiteboard: [needs review dbaron][needs sr bsmedberg] → [needs review bsmedberg][needs sr dbaron]
(Assignee)

Updated

10 years ago
Blocks: 489707
Comment on attachment 374189 [details] [diff] [review]
v1.1

sr=dbaron assuming this passes the try server

You might want to extend the comment to note that |rhs| may be T** or I** where I is a base class of T.
Attachment #374189 - Flags: superreview?(dbaron) → superreview+

Comment 6

10 years ago
(In reply to comment #3)
> If we ever do the same thing for nsCOMPtr (which might be sensible given that
> interfaces can inherit from other interfaces), we should have the equivalent of
> an NSCAP_ASSERT_NO_QUERY_NEEDED on the result.

Yeah, I've been hitting this case with nsCOMPtr more and more. Working with nsIWritablePropertyBag and returning it as an nsIPropertyBag is one example. Songbird also has a fair number of interface inheritance so we hit it a fair bit.
(Assignee)

Updated

10 years ago
Whiteboard: [needs review bsmedberg][needs sr dbaron] → [needs review bsmedberg]
Target Milestone: --- → mozilla1.9.2a1

Updated

10 years ago
Attachment #374189 - Flags: review?(benjamin) → review+
(Assignee)

Updated

10 years ago
Whiteboard: [needs review bsmedberg] → [can land]
(Assignee)

Comment 7

10 years ago
Updated the comment per comment 5 before pushing.
http://hg.mozilla.org/mozilla-central/rev/ddb8d3efc243
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
Component: XP Toolkit/Widgets: XUL → XPCOM
QA Contact: xptoolkit.xul → xpcom

Comment 8

10 years ago
Note that nsCOMPtr is much harder because of refcnt balancing.
You need to log in before you can comment on or make changes to this bug.