Re: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-08 Thread Aryeh Gregor
On Tue, Jul 7, 2015 at 6:10 PM, Michael Layzell mich...@thelayzells.com wrote: No, I never checked if it happens on an optimized build, but as C++ follows an as-if principal, which means that code has to execute as if those temporaries had been created. Unfortunately, AddRef and Release are

Re: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-08 Thread Seth Fowler
I brought this up on IRC, but I think this is worth taking to the list: Replacing TemporaryRef with already_AddRefed has already caused at least one leak because already_AddRefed does not call Release() on the pointer it holds if nothing takes ownership of that pointer before its destructor

Re: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-08 Thread Kyle Huey
On Tue, Jul 7, 2015 at 9:59 AM, Seth Fowler mfow...@mozilla.com wrote: I brought this up on IRC, but I think this is worth taking to the list: Replacing TemporaryRef with already_AddRefed has already caused at least one leak because already_AddRefed does not call Release() on the pointer it

Re: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-08 Thread Michael Layzell
We're already working on a static analysis in https://bugzilla.mozilla.org/show_bug.cgi?id=1180993 which will prevent code like that from passing try. Hopefully that will help with your concerns. On 2015-07-07 12:59 PM, Seth Fowler wrote: I brought this up on IRC, but I think this is worth

Re: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-07 Thread Michael Layzell
On 2015-07-07 6:37 AM, Aryeh Gregor wrote: Did you check whether this actually occurs in an optimized build? C++ allows temporaries to be optimized away under some circumstances, e.g., when returning a local variable. It would make a lot of sense to me if it allowed the temporary created by a

Re: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-07 Thread Aryeh Gregor
On Fri, Jul 3, 2015 at 6:22 PM, Michael Layzell mich...@thelayzells.com wrote: So the ternary actually causes an unnecessary AddRef/Release pair, neat. Did you check whether this actually occurs in an optimized build? C++ allows temporaries to be optimized away under some circumstances, e.g.,

Re: Fwd: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-06 Thread Neil
Michael Layzell wrote: In summary, the nsRefPtr is copied into a temporary in its side of the conditional. The nullptr is cast to a struct Foo*, which is constructed into a nsRefPtr, which is bound to a temporary, and then moved around a bit between temporaries. The resulting temporary

Re: Fwd: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-06 Thread Michael Layzell
I suppose that the general problem is that we are using an nsRefPtr lvalue in one side of the branch, and an nsRefPtr xvalue on the other (as the nullptr is being cast to an nsRefPtr, which has to live in a temporary making it an xvalue). This is reasonable in a situation where we actually

Re: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-05 Thread Aryeh Gregor
On Fri, Jul 3, 2015 at 3:56 PM, Neil n...@parkwaycc.co.uk wrote: Aryeh Gregor wrote: we still want a new type for function parameters that accepts implicit conversions from nsRefPtr/nsCOMPtr, to use instead of raw pointers. Sure, but that won't stop someone from writing ArgFoo foo =

Fwd: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-03 Thread Michael Layzell
With regard to what #2 is doing, I threw the following into a file with nsRefPtr, and got clang to dump the AST: struct Foo {void AddRef() {} void Release() {}}; nsRefPtrFoo bar; void foo() { Foo* x = true ? nullptr : bar; } `-FunctionDecl 0x103805750 line:943:1, line:945:1 line:943:6 foo

Re: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-03 Thread Michael Layzell
On Fri, Jul 3, 2015 at 8:56 AM, Neil n...@parkwaycc.co.uk wrote: Sure, but that won't stop someone from writing ArgFoo foo = ReturnFoo2(); will it? Something like that is fairly trivial for a static analysis system like our clang plugin to catch, if we want to create a type like that.

Re: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-03 Thread Neil
Nathan Froyd wrote: I guess we could fix this in current m-c to be smarter about the parameter passing, perhaps by declaring that ParameterStoragensCOMPtrT is StoreCopyPassByConstLRefnsCOMPtrT? (Similarly for nsRefPtrT.) So what's StorensRefPtrPassByPtr for? -- Warning: May contain traces

Re: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-03 Thread Neil
Aryeh Gregor wrote: we still want a new type for function parameters that accepts implicit conversions from nsRefPtr/nsCOMPtr, to use instead of raw pointers. Sure, but that won't stop someone from writing ArgFoo foo = ReturnFoo2(); will it? -- Warning: May contain traces of nuts.

Re: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-03 Thread Aryeh Gregor
On Thu, Jul 2, 2015 at 3:50 PM, Neil n...@parkwaycc.co.uk wrote: Would you mind reminding me what the failure case this avoids is? already_AddRefedFoo ReturnFoo1() { nsRefPtrFoo foo = new Foo(); return foo.forget(); } nsRefPtrFoo ReturnFoo2() { return new Foo(); } // This doesn't compile

Re: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-02 Thread Nathan Froyd
On Thu, Jul 2, 2015 at 9:21 AM, Neil n...@parkwaycc.co.uk wrote: Nathan Froyd wrote: I tried this, fixed a few compilation errors, then decided this wasn't worth it just yet and threw my work into a bug. Comments appreciated: https://bugzilla.mozilla.org/show_bug.cgi?id=1179451 1.

Re: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-02 Thread Neil
Nathan Froyd wrote: I tried this, fixed a few compilation errors, then decided this wasn't worth it just yet and threw my work into a bug. Comments appreciated: https://bugzilla.mozilla.org/show_bug.cgi?id=1179451 1. It's because QueryInterface has to addref, so we can't directly

Re: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-02 Thread Neil
Jeff Muizelaar wrote: I believe this is predicated on removing the implicit conversion from nsRefPtrT to T* Would you mind reminding me what the failure case this avoids is? -- Warning: May contain traces of nuts. ___ dev-platform mailing list

Re: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-01 Thread Aryeh Gregor
On Wed, Jul 1, 2015 at 6:24 AM, Joshua Cranmer  pidgeo...@gmail.com wrote: You'd get the same benefit, I think, by making operator T*() = delete;, syntax which is accepted on gcc 4.8.1+, clang, and MSVC 2015 IIRC. I once tried this and found it had problematic side effects that made

Re: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-07-01 Thread Nathan Froyd
On Wed, Jul 1, 2015 at 7:39 AM, Aryeh Gregor a...@aryeh.name wrote: On Wed, Jul 1, 2015 at 6:24 AM, Joshua Cranmer  pidgeo...@gmail.com wrote: You'd get the same benefit, I think, by making operator T*() = delete;, syntax which is accepted on gcc 4.8.1+, clang, and MSVC 2015 IIRC. I once

mozilla::TemporaryRef is gone; please use already_AddRefed

2015-06-30 Thread Nathan Froyd
Bug 1161627 has landed on inbound, which converts all uses of mozilla::TemporaryRef to already_AddRefed and removes TemporaryRef. (already_AddRefed moved to MFBT several months ago, in case you were wondering about the spreading of XPCOM concepts.) TemporaryRef was added for easier porting of

Re: mozilla::TemporaryRef is gone; please use already_AddRefed

2015-06-30 Thread Robert O'Callahan
Will it ever be possible to eliminate TemporaryRef and use moves instead? Rob -- oIo otoeololo oyooouo otohoaoto oaonoyooonoeo owohooo oioso oaonogoroyo owoiotoho oao oboroootohoeoro oooro osoiosotoeoro owoiololo oboeo osouobojoeocoto otooo ojouodogomoeonoto.o oAogoaoiono,o oaonoyooonoeo owohooo