Re: Getting rid of already_AddRefed?

On 9/12/16 4:21 PM, Jim Blandy wrote: So, in this pattern, if the target thread's execution of SomeRunnable::Run could fail, causing ~SomeRunnable to call mMyRef's destructor and thus free the referent on the target thread, that would be a bug? Yes, it would. -Boris

Re: Getting rid of already_AddRefed?

So, in this pattern, if the target thread's execution of SomeRunnable::Run could fail, causing ~SomeRunnable to call mMyRef's destructor and thus free the referent on the target thread, that would be a bug? On Mon, Sep 12, 2016 at 12:47 PM, Boris Zbarsky wrote: > On 9/12/16

Re: Getting rid of already_AddRefed?

On 9/12/16 3:40 PM, Jim Blandy wrote: Could you go into more detail here? Either the caller will free it, or the callee will free it --- but they're both on the same thread. We have this pretty common pattern for handing references around threads safely: Main thread: RefPtr runnable =

Re: Getting rid of already_AddRefed?

On Mon, Sep 12, 2016 at 12:22 PM, Boris Zbarsky wrote: It brings up one potential concern with Move: since the > >> callee might not take the value (intentionally or unintentionally), the >> caller must *refrain* from caring about the state of its Move()'d >> variable, between

Re: Getting rid of already_AddRefed?

On 09/12/2016 12:22 PM, Boris Zbarsky wrote: > It's worse than that. For a wide range of callers (anyone handing the > ref across threads), the caller must check immediately whether the > callee actually took the value and if not make sure things are released > on the proper thread... Ah, ok; I

Re: Getting rid of already_AddRefed?

On 9/12/16 3:18 PM, Daniel Holbert wrote: (1) They guard against leaks from unused already_AddRefed. (2) They guard against logic errors where we should've taken the value but forgot to do so. (And it enforces that callees must take the value, even if they have some error handling cases where

Re: Getting rid of already_AddRefed?

On 09/12/2016 11:00 AM, Boris Zbarsky wrote: > On 9/12/16 1:53 PM, Daniel Holbert wrote: >> (I believe we have the "already_AddRefed as a parameter" pattern in our >> code in order to avoid an extra addref/release when ownership is being >> transferred into a function. > > And assertions if the

Re: Getting rid of already_AddRefed?

On 9/12/16 1:53 PM, Daniel Holbert wrote: (I believe we have the "already_AddRefed as a parameter" pattern in our code in order to avoid an extra addref/release when ownership is being transferred into a function. And assertions if the function does not actually take the reference... But,

Re: Getting rid of already_AddRefed?

On Mon, Sep 12, 2016 at 10:53 AM, Daniel Holbert wrote: > On 08/12/2014 07:59 AM, Benjamin Smedberg wrote: >> But now that nsCOMPtr/nsRefPtr support proper move constructors, [...] >> Could we replace every already_AddRefed return value with a nsCOMPtr? > > I have a variant

Re: Getting rid of already_AddRefed?

On 08/12/2014 07:59 AM, Benjamin Smedberg wrote: > But now that nsCOMPtr/nsRefPtr support proper move constructors, [...] > Could we replace every already_AddRefed return value with a nsCOMPtr? I have a variant of bsmedberg's original question here. He asked about return values, but I'm

Re: Getting rid of already_AddRefed?

On Sat, Dec 27, 2014 at 12:34 AM, smaug sm...@welho.com wrote: How would this setup help with the case when one passes nsCOMPtr/nsRefPtr member variable as a param? I believe that has been the most common issue with caller-should-keep-the-parameter-alive - one just doesn't remember to make

Re: Getting rid of already_AddRefed?

On Sat, Dec 27, 2014 at 5:54 AM, Karl Tomlinson mozn...@karlt.net wrote: Aryeh Gregor writes: Do we have a better convention for an in/out parameter that's a pointer to a refcounted class? editor uses this convention in a number of functions for pass me a node/pointer pair as input, and

Re: Getting rid of already_AddRefed?

On Mon, Dec 22, 2014 at 11:10 PM, Jeff Muizelaar jmuizel...@mozilla.com wrote: Possible solutions would be to: - remove implicit conversions to T* If this were done, I think we should change the calling convention for functions that take pointers to refcounted classes. The convention is

Re: Getting rid of already_AddRefed?

On 12/26/2014 03:08 PM, Aryeh Gregor wrote: On Mon, Dec 22, 2014 at 11:10 PM, Jeff Muizelaar jmuizel...@mozilla.com wrote: Possible solutions would be to: - remove implicit conversions to T* If this were done, I think we should change the calling convention for functions that take pointers

Re: Getting rid of already_AddRefed?

On Tue, Dec 23, 2014 at 1:21 AM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: Are there good use cases for having functions accept an nsRefPtrT? If not, we can outlaw them. Aryeh Gregor writes: Do we have a better convention for an in/out parameter that's a pointer to a refcounted class?

Re: Getting rid of already_AddRefed?

On 2014-12-23 2:52 AM, Ms2ger wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 12/23/2014 12:45 AM, Ehsan Akhgari wrote: On 2014-12-22 6:35 PM, L. David Baron wrote: On Monday 2014-12-22 18:21 -0500, Ehsan Akhgari wrote: On 2014-12-22 6:07 PM, L. David Baron wrote: On Monday

Re: Getting rid of already_AddRefed?

On 2014-12-22 6:52 PM, Eric Rescorla wrote: On Mon, Dec 22, 2014 at 3:35 PM, L. David Baron dba...@dbaron.org mailto:dba...@dbaron.org wrote: On Monday 2014-12-22 18:21 -0500, Ehsan Akhgari wrote: On 2014-12-22 6:07 PM, L. David Baron wrote: On Monday 2014-12-22 17:54 -0500,

Re: Getting rid of already_AddRefed?

On 2014-12-23 9:47 AM, Ehsan Akhgari wrote: On 2014-12-23 2:52 AM, Ms2ger wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 12/23/2014 12:45 AM, Ehsan Akhgari wrote: On 2014-12-22 6:35 PM, L. David Baron wrote: On Monday 2014-12-22 18:21 -0500, Ehsan Akhgari wrote: On 2014-12-22 6:07

Re: Getting rid of already_AddRefed?

On Tue, Dec 23, 2014 at 6:48 AM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: On 2014-12-22 6:52 PM, Eric Rescorla wrote: On Mon, Dec 22, 2014 at 3:35 PM, L. David Baron dba...@dbaron.org mailto:dba...@dbaron.org wrote: On Monday 2014-12-22 18:21 -0500, Ehsan Akhgari wrote: On

Re: Getting rid of already_AddRefed?

On 2014-12-23 10:38 AM, Eric Rescorla wrote: On Tue, Dec 23, 2014 at 6:48 AM, Ehsan Akhgari ehsan.akhg...@gmail.com mailto:ehsan.akhg...@gmail.com wrote: On 2014-12-22 6:52 PM, Eric Rescorla wrote: On Mon, Dec 22, 2014 at 3:35 PM, L. David Baron dba...@dbaron.org

Re: Getting rid of already_AddRefed?

On Tuesday 2014-12-23 08:36 -0800, Eric Rescorla wrote: Why not pass the raw pointer to the function? My general theory is that smart pointers, once boxed, should never be unboxed. The major arguments I see for unboxing is performance, but if you pass a ref, then you don't have the

Re: Getting rid of already_AddRefed?

On 2014-12-23 11:36 AM, Eric Rescorla wrote: On Tue, Dec 23, 2014 at 8:32 AM, Ehsan Akhgari ehsan.akhg...@gmail.com mailto:ehsan.akhg...@gmail.com wrote: On 2014-12-23 10:38 AM, Eric Rescorla wrote: On Tue, Dec 23, 2014 at 6:48 AM, Ehsan Akhgari ehsan.akhg...@gmail.com

Re: Getting rid of already_AddRefed?

On Tue, Dec 23, 2014 at 8:48 AM, L. David Baron dba...@dbaron.org wrote: On Tuesday 2014-12-23 08:36 -0800, Eric Rescorla wrote: Why not pass the raw pointer to the function? My general theory is that smart pointers, once boxed, should never be unboxed. The major arguments I see for

Re: Getting rid of already_AddRefed?

On Tue, Dec 23, 2014 at 8:53 AM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: On 2014-12-23 11:36 AM, Eric Rescorla wrote: On Tue, Dec 23, 2014 at 8:32 AM, Ehsan Akhgari ehsan.akhg...@gmail.com mailto:ehsan.akhg...@gmail.com wrote: On 2014-12-23 10:38 AM, Eric Rescorla wrote:

Re: Getting rid of already_AddRefed?

On Tuesday 2014-12-23 09:49 -0800, Eric Rescorla wrote: Hmm... This seems to work fine with RefPtr, but it *doesn't* work with nsRefPtr (see end of message for the test code). I'm guessing because of: http://dxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h?from=RefPtrcase=true#239 But

Re: Getting rid of already_AddRefed?

On Tue, Dec 23, 2014 at 9:55 AM, L. David Baron dba...@dbaron.org wrote: But that's an implicit constructor that's causing extra refcount traffic, which is one of the things we didn't want here. I don't think that it's quite fair to complain about implicit conversion if the whole point is to

Re: Getting rid of already_AddRefed?

On 2014-12-23 12:51 PM, Eric Rescorla wrote: On Tue, Dec 23, 2014 at 8:53 AM, Ehsan Akhgari ehsan.akhg...@gmail.com mailto:ehsan.akhg...@gmail.com wrote: On 2014-12-23 11:36 AM, Eric Rescorla wrote: On Tue, Dec 23, 2014 at 8:32 AM, Ehsan Akhgari ehsan.akhg...@gmail.com

Re: Getting rid of already_AddRefed?

On Tuesday 2014-12-23 09:59 -0800, Martin Thomson wrote: On Tue, Dec 23, 2014 at 9:55 AM, L. David Baron dba...@dbaron.org wrote: But that's an implicit constructor that's causing extra refcount traffic, which is one of the things we didn't want here. I don't think that it's quite

Re: Getting rid of already_AddRefed?

On 12/23/2014 10:48 AM, L. David Baron wrote: Our convention has always been to pass raw pointers, generally with the assumption that the caller is expected to ensure the pointer lives across the function call. Like Eric, I would like us to move away from this convention over time, or at

Re: Getting rid of already_AddRefed?

On Tue, Dec 23, 2014 at 10:14 AM, L. David Baron dba...@dbaron.org wrote: On Tuesday 2014-12-23 09:59 -0800, Martin Thomson wrote: On Tue, Dec 23, 2014 at 9:55 AM, L. David Baron dba...@dbaron.org wrote: But that's an implicit constructor that's causing extra refcount traffic, which

Re: Getting rid of already_AddRefed?

On Tue, Dec 23, 2014 at 1:03 PM, Jeff Walden jwalden+...@mit.edu wrote: On 12/23/2014 10:48 AM, L. David Baron wrote: Our convention has always been to pass raw pointers, generally with the assumption that the caller is expected to ensure the pointer lives across the function call. Like

Re: Getting rid of already_AddRefed?

On Tuesday 2014-12-23 13:14 -0800, Eric Rescorla wrote: Just to be clear, is your problem the implicit conversion itself or the reference count increment/decrement? The latter -- the problem is that there's an implicit conversion that has surprising side-effects. (It sounds from this

Re: Getting rid of already_AddRefed?

On Tue, Dec 23, 2014 at 1:51 PM, L. David Baron dba...@dbaron.org wrote: Just to be clear, is your problem the implicit conversion itself or the reference count increment/decrement? The latter -- the problem is that there's an implicit conversion that has surprising side-effects. Why are

Re: Getting rid of already_AddRefed?

On Tue, Dec 23, 2014 at 1:51 PM, L. David Baron dba...@dbaron.org wrote: On Tuesday 2014-12-23 13:14 -0800, Eric Rescorla wrote: Just to be clear, is your problem the implicit conversion itself or the reference count increment/decrement? The latter -- the problem is that there's an

Re: Getting rid of already_AddRefed?

On Tuesday 2014-12-23 13:59 -0800, Martin Thomson wrote: Why are you surprised that when you pass a pointer to a function that the function gets a pointer? You're putting words in my mouth. I agree that the performance is less than perfect. I do have to ask: at what point do you consider the

Re: Getting rid of already_AddRefed?

On Tue, Dec 23, 2014 at 2:07 PM, L. David Baron dba...@dbaron.org wrote: On Tuesday 2014-12-23 14:03 -0800, Eric Rescorla wrote: This may be a much longer argument, but I'm not convinced that sacrificing what would otherwise be good programming practice (never unboxing your pointers) at

Re: Getting rid of already_AddRefed?

On 2014-12-23 5:13 PM, Eric Rescorla wrote: On Tue, Dec 23, 2014 at 2:07 PM, L. David Baron dba...@dbaron.org mailto:dba...@dbaron.org wrote: On Tuesday 2014-12-23 14:03 -0800, Eric Rescorla wrote: This may be a much longer argument, but I'm not convinced that sacrificing what

Re: Getting rid of already_AddRefed?

On 2014-12-23 5:30 PM, Eric Rescorla wrote: On Tue, Dec 23, 2014 at 2:19 PM, Ehsan Akhgari ehsan.akhg...@gmail.com mailto:ehsan.akhg...@gmail.com wrote: On 2014-12-23 5:13 PM, Eric Rescorla wrote: On Tue, Dec 23, 2014 at 2:07 PM, L. David Baron dba...@dbaron.org

Re: Getting rid of already_AddRefed?

We were talking about this problem and it was a bunch of work to figure out the conclusion so I decided to write a summary: Replacing already_AddRefed with nsRefPtr causes allows two new things: nsRefPtrT getT(); 1. T* p = getT(); // this is unsafe because the destructor runs immediately and

Re: Getting rid of already_AddRefed?

On 2014-12-22 4:10 PM, Jeff Muizelaar wrote: We were talking about this problem and it was a bunch of work to figure out the conclusion so I decided to write a summary: Replacing already_AddRefed with nsRefPtr causes allows two new things: nsRefPtrT getT(); 1. T* p = getT(); // this is

Re: Getting rid of already_AddRefed?

On Mon, Dec 22, 2014 at 1:12 PM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: On 2014-12-22 4:10 PM, Jeff Muizelaar wrote: We were talking about this problem and it was a bunch of work to figure out the conclusion so I decided to write a summary: Replacing already_AddRefed with nsRefPtr

Re: Getting rid of already_AddRefed?

On Monday 2014-12-22 16:10 -0500, Jeff Muizelaar wrote: We were talking about this problem and it was a bunch of work to figure out the conclusion so I decided to write a summary: Replacing already_AddRefed with nsRefPtr causes allows two new things: nsRefPtrT getT(); 1. T* p = getT();

Re: Getting rid of already_AddRefed?

On 2014-12-22 4:56 PM, L. David Baron wrote: On Monday 2014-12-22 16:10 -0500, Jeff Muizelaar wrote: We were talking about this problem and it was a bunch of work to figure out the conclusion so I decided to write a summary: Replacing already_AddRefed with nsRefPtr causes allows two new

Re: Getting rid of already_AddRefed?

On Monday 2014-12-22 17:54 -0500, Ehsan Akhgari wrote: On 2014-12-22 4:56 PM, L. David Baron wrote: I think removing implicit conversions to T* will make a lot of code in the tree uglier (.get() everywhere). That might, in turn, encourage people to do worse things to avoid having to write

Re: Getting rid of already_AddRefed?

On Mon, Dec 22, 2014 at 04:56:12PM -0500, L. David Baron wrote: (And, on that subject, I think development practice in MFBT has been too readily adding new and different things instead of moving the existing things from XPCOM into MFBT and then improving them incrementally.) That essentially

Re: Getting rid of already_AddRefed?

On 2014-12-22 6:07 PM, L. David Baron wrote: On Monday 2014-12-22 17:54 -0500, Ehsan Akhgari wrote: On 2014-12-22 4:56 PM, L. David Baron wrote: I think removing implicit conversions to T* will make a lot of code in the tree uglier (.get() everywhere). That might, in turn, encourage people to

Re: Getting rid of already_AddRefed?

On Monday 2014-12-22 18:21 -0500, Ehsan Akhgari wrote: On 2014-12-22 6:07 PM, L. David Baron wrote: On Monday 2014-12-22 17:54 -0500, Ehsan Akhgari wrote: On 2014-12-22 4:56 PM, L. David Baron wrote: I think removing implicit conversions to T* will make a lot of code in the tree uglier

Re: Getting rid of already_AddRefed?

On 2014-12-22 6:35 PM, L. David Baron wrote: On Monday 2014-12-22 18:21 -0500, Ehsan Akhgari wrote: On 2014-12-22 6:07 PM, L. David Baron wrote: On Monday 2014-12-22 17:54 -0500, Ehsan Akhgari wrote: On 2014-12-22 4:56 PM, L. David Baron wrote: I think removing implicit conversions to T*

Re: Getting rid of already_AddRefed?

On Mon, Dec 22, 2014 at 3:35 PM, L. David Baron dba...@dbaron.org wrote: On Monday 2014-12-22 18:21 -0500, Ehsan Akhgari wrote: On 2014-12-22 6:07 PM, L. David Baron wrote: On Monday 2014-12-22 17:54 -0500, Ehsan Akhgari wrote: On 2014-12-22 4:56 PM, L. David Baron wrote: I think

Re: Getting rid of already_AddRefed?

-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 12/23/2014 12:45 AM, Ehsan Akhgari wrote: On 2014-12-22 6:35 PM, L. David Baron wrote: On Monday 2014-12-22 18:21 -0500, Ehsan Akhgari wrote: On 2014-12-22 6:07 PM, L. David Baron wrote: On Monday 2014-12-22 17:54 -0500, Ehsan Akhgari wrote:

Re: Getting rid of already_AddRefed?

Aryeh Gregor wrote: for instance, here's a real-world bit of code from nsWSRunObject: if ((aRun-mRightType WSType::block) IsBlockNode(nsCOMPtrnsINode(GetWSBoundingParent( { GetWSBoundingParent() returns an already_AddRefednsINode Well there's your problem:

Re: Getting rid of already_AddRefed?

On Thu, Aug 14, 2014 at 12:00 PM, Neil n...@parkwaycc.co.uk wrote: Well there's your problem: GetWSBoundingParent doesn't need to own the nodes it works on. Editing code is ancient, poorly maintained, and not performance-sensitive, so personally, I don't ever use raw pointers as local variables

Re: Getting rid of already_AddRefed?

On Thu, Aug 14, 2014 at 10:02 AM, Aryeh Gregor a...@aryeh.name wrote: On Thu, Aug 14, 2014 at 12:00 PM, Neil n...@parkwaycc.co.uk wrote: Well there's your problem: GetWSBoundingParent doesn't need to own the nodes it works on. Editing code is ancient, poorly maintained, and not

Re: Getting rid of already_AddRefed?

On 08/12/2014 10:06 AM, Ehsan Akhgari wrote: It could also be solved with making operator T*() explicit, but neither of these options are something that we can use in our code base today. So at risk of adding yet another flavor of thing: why not introduce an already_AddRefedT sort of struct

Re: Getting rid of already_AddRefed?

Karl Tomlinson wrote: Aryeh Gregor writes: The compiler is required to use the move constructor (if one exists) instead of the copy constructor when constructing the return value of a function, and also when initializing an object from the return value of a function, or assigning the

Re: Getting rid of already_AddRefed?

On Wed, Aug 13, 2014 at 9:42 AM, Jeff Walden jwalden+...@mit.edu wrote: So at risk of adding yet another flavor of thing: why not introduce an already_AddRefedT sort of struct that *does* own an addref, *will* release on destruction if not nulled out, and does *not* explicitly convert to T*?

Re: Getting rid of already_AddRefed?

On 08/12/2014 06:23 PM, Aryeh Gregor wrote: On Tue, Aug 12, 2014 at 6:16 PM, Benoit Jacob jacob.benoi...@gmail.com wrote: As far as I know, the only downside in replacing already_AddRefed by nsCOMPtr would be to incur more useless calls to AddRef and Release. In the case of threadsafe i.e.

Re: Getting rid of already_AddRefed?

On Wed, Aug 13, 2014 at 4:11 PM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: What goal would this achieve? I don't understand why it's OK to ignore the return value of a function which returns an already AddRef'ed object from a conceptual perspective. You might want the effect of the

Re: Getting rid of already_AddRefed?

On Wed, Aug 13, 2014 at 9:32 AM, Aryeh Gregor a...@aryeh.name wrote: On Wed, Aug 13, 2014 at 4:11 PM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: What goal would this achieve? I don't understand why it's OK to ignore the return value of a function which returns an already AddRef'ed

Re: Getting rid of already_AddRefed?

On Wed, Aug 13, 2014 at 5:44 PM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: Can't you do the following instead? unused MyFunction(); // I know that I'm leaking this ref, but it's ok somehow No, because the use-case is where you don't want to leak the ref -- you want it to be released

Re: Getting rid of already_AddRefed?

On Wed, Aug 13, 2014 at 5:14 AM, Aryeh Gregor a...@aryeh.name wrote: I think this can be improved upon a bit further: just change already_AddRefed to behave more similarly to nsCOMPtr, but still not convert to T* implicitly. So for instance: * Change ~already_AddRefed to just release the

Re: Getting rid of already_AddRefed?

On Wed, Aug 13, 2014 at 9:24 AM, Aryeh Gregor a...@aryeh.name wrote: No, because the use-case is where you don't want to leak the ref -- you want it to be released automatically for you. So for instance, here's a real-world bit of code from nsWSRunObject: if ((aRun-mRightType

Re: Getting rid of already_AddRefed?

On 08/13/2014 07:44 AM, Ehsan Akhgari wrote: If that is the goal, then I don't agree that is a useful outcome at all. I *do* wish that there were better *and* safer ways of doing more things automatically but ownership transfers are inherently unsafe operations that are expressed using

Re: Getting rid of already_AddRefed?

On 2014-08-13, 12:24 PM, Aryeh Gregor wrote: On Wed, Aug 13, 2014 at 5:44 PM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: Can't you do the following instead? unused MyFunction(); // I know that I'm leaking this ref, but it's ok somehow No, because the use-case is where you don't want to

Re: Getting rid of already_AddRefed?

On 08/13/2014 07:24 PM, Aryeh Gregor wrote: On Wed, Aug 13, 2014 at 5:44 PM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: Can't you do the following instead? unused MyFunction(); // I know that I'm leaking this ref, but it's ok somehow No, because the use-case is where you don't want to

Getting rid of already_AddRefed?

Just reading bug 1052477, and I'm wondering about what are intentions are for already_AddRefed. In that bug it's proposed to change the return type of NS_NewAtom from already_AddRefed to nsCOMPtr. I don't think that actually saves any addref/release pairs if done properly, since you'd

Re: Getting rid of already_AddRefed?

On Tue, Aug 12, 2014 at 5:59 PM, Benjamin Smedberg benja...@smedbergs.us wrote: Just reading bug 1052477, and I'm wondering about what are intentions are for already_AddRefed. In that bug it's proposed to change the return type of NS_NewAtom from already_AddRefed to nsCOMPtr. I don't think

Re: Getting rid of already_AddRefed?

On Tuesday 2014-08-12 10:59 -0400, Benjamin Smedberg wrote: Just reading bug 1052477, and I'm wondering about what are intentions are for already_AddRefed. In that bug it's proposed to change the return type of NS_NewAtom from already_AddRefed to nsCOMPtr. I don't think that actually saves

Re: Getting rid of already_AddRefed?

As far as I know, the only downside in replacing already_AddRefed by nsCOMPtr would be to incur more useless calls to AddRef and Release. In the case of threadsafe i.e. atomic refcounting, these use atomic instructions, which might be expensive enough on certain ARM CPUs that this might matter. So

Re: Getting rid of already_AddRefed?

On Tue, Aug 12, 2014 at 6:14 PM, L. David Baron dba...@dbaron.org wrote: Bug 967364 did already add assertions to guarantee this. Yes, if I'm not mistaken, already_AddRefed these days is quite safe. Two thoughts: (1) I think it introduces a somewhat major coding style risk, in that it

Re: Getting rid of already_AddRefed?

On Tue, Aug 12, 2014 at 6:16 PM, Benoit Jacob jacob.benoi...@gmail.com wrote: As far as I know, the only downside in replacing already_AddRefed by nsCOMPtr would be to incur more useless calls to AddRef and Release. In the case of threadsafe i.e. atomic refcounting, these use atomic

Re: Getting rid of already_AddRefed?

On Tuesday 2014-08-12 18:15 +0300, Aryeh Gregor wrote: 1) You can use the return value directly without assigning it to an nsCOMPtr/nsRefPtr. For instance, pass it to a function that wants a raw pointer, or compare it against a different value. I consider this a disadvantage, as I pointed out

Re: Getting rid of already_AddRefed?

On Tuesday 2014-08-12 18:22 +0300, Aryeh Gregor wrote: On Tue, Aug 12, 2014 at 6:14 PM, L. David Baron dba...@dbaron.org wrote: Two thoughts: (1) I think it introduces a somewhat major coding style risk, in that it makes it possible to accidentally assign the value of a function that

Re: Getting rid of already_AddRefed?

On Tue, Aug 12, 2014 at 6:25 PM, L. David Baron dba...@dbaron.org wrote: On Tuesday 2014-08-12 18:15 +0300, Aryeh Gregor wrote: 1) You can use the return value directly without assigning it to an 4) If the callee already holds a strong reference in a local variable, it can just return that

Re: Getting rid of already_AddRefed?

On Tuesday 2014-08-12 18:40 +0300, Aryeh Gregor wrote: On Tue, Aug 12, 2014 at 6:29 PM, L. David Baron dba...@dbaron.org wrote: This is done very commonly when we know some other data structure (or the lifetime of |this|) owns the object, and will continue to across the lifetime of the

Re: Getting rid of already_AddRefed?

On Tuesday, August 12, 2014 11:22:05 AM UTC-4, Aryeh Gregor wrote: For refcounted types, isn't a raw pointer in a local variable a red flag to reviewers to begin with? If GetT() returns a raw pointer today, like nsINode::GetFirstChild() or something, storing the result in a raw pointer is a

Re: Getting rid of already_AddRefed?

On 8/12/2014 9:59 AM, Benjamin Smedberg wrote: Just reading bug 1052477, and I'm wondering about what are intentions are for already_AddRefed. In that bug it's proposed to change the return type of NS_NewAtom from already_AddRefed to nsCOMPtr. I don't think that actually saves any

Re: Getting rid of already_AddRefed?

On Tue, Aug 12, 2014 at 6:46 PM, L. David Baron dba...@dbaron.org wrote: In these cases we document that it's likely safe for callers to do this by having those getters return raw pointers. Getters that require reference-counting return already_AddRefed. Thus the designer of the API makes a

Re: Getting rid of already_AddRefed?

On 8/12/2014 12:28 PM, Joshua Cranmer  wrote: But now that nsCOMPtr/nsRefPtr support proper move constructors, is there any reason for already_AddRefed to exist at all in our codebase? Could we replace every already_AddRefed return value with a nsCOMPtr? The rationale for why we still

Re: Getting rid of already_AddRefed?

On Tue, Aug 12, 2014 at 9:35 AM, Aryeh Gregor a...@aryeh.name wrote: On Tue, Aug 12, 2014 at 6:46 PM, L. David Baron dba...@dbaron.org wrote: In these cases we document that it's likely safe for callers to do this by having those getters return raw pointers. Getters that require

Re: Getting rid of already_AddRefed?

On Tue, Aug 12, 2014 at 7:12 PM, Vladimir Vukicevic vladi...@pobox.com wrote: It's unfortunate that we can't create a nsCOMPtr that will disallow assignment to a bare pointer without an explicit .get(), but will still allow conversion to a bare pointer for arg passing purposes. (Or can we? I

Re: Getting rid of already_AddRefed?

On Tue, Aug 12, 2014 at 7:37 PM, Benjamin Smedberg benja...@smedbergs.us wrote: On 8/12/2014 12:28 PM, Joshua Cranmer  wrote: The rationale for why we still had it was that: nsIFoo *foobar = ReturnsACOMPtr(); silently leaks. Really? I thought that in this case there would be no leak

Re: Getting rid of already_AddRefed?

On 8/12/2014 11:12 AM, Vladimir Vukicevic wrote: It's unfortunate that we can't create a nsCOMPtr that will disallow assignment to a bare pointer without an explicit .get(), but will still allow conversion to a bare pointer for arg passing purposes. (Or can we? I admit my C++-fu is not that

Re: Getting rid of already_AddRefed?

On 8/12/2014 11:40 AM, Aryeh Gregor wrote: On Tue, Aug 12, 2014 at 7:37 PM, Benjamin Smedberg benja...@smedbergs.us wrote: On 8/12/2014 12:28 PM, Joshua Cranmer  wrote: The rationale for why we still had it was that: nsIFoo *foobar = ReturnsACOMPtr(); silently leaks. Really? I thought that

Re: Getting rid of already_AddRefed?

On 2014-08-12, 12:41 PM, Joshua Cranmer  wrote: On 8/12/2014 11:12 AM, Vladimir Vukicevic wrote: It's unfortunate that we can't create a nsCOMPtr that will disallow assignment to a bare pointer without an explicit .get(), but will still allow conversion to a bare pointer for arg passing

Re: Getting rid of already_AddRefed?

On 2014-08-12, 11:25 AM, L. David Baron wrote: On Tuesday 2014-08-12 18:15 +0300, Aryeh Gregor wrote: 1) You can use the return value directly without assigning it to an nsCOMPtr/nsRefPtr. For instance, pass it to a function that wants a raw pointer, or compare it against a different value.

Re: Getting rid of already_AddRefed?

Aryeh Gregor wrote: 2) It's easier to use, as evidenced by the patches in bugs 1015114 and 1052477. No .forget() is needed, a raw pointer can be returned directly, even things like do_QueryInterface can be returned directly without a temporary nsCOMPtr (since the return type creates the

Re: Getting rid of already_AddRefed?

On Wed, Aug 13, 2014 at 4:35 AM, Aryeh Gregor a...@aryeh.name wrote: On Tue, Aug 12, 2014 at 6:46 PM, L. David Baron dba...@dbaron.org wrote: In these cases we document that it's likely safe for callers to do this by having those getters return raw pointers. Getters that require

Re: Getting rid of already_AddRefed?

Aryeh Gregor writes: The compiler is required to use the move constructor (if one exists) instead of the copy constructor when constructing the return value of a function, and also when initializing an object from the return value of a function, or assigning the return value of a function.