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 that actually saves any > addref/release pairs if done properly, since you'd typically .forget() into > the return value anyway. But it does make it slightly safer at callsites, > because the compiler will guarantee that the return value is always released > instead of us relying on every already_AddRefed being saved into a nsCOMPtr.
Correct, it doesn't save addref/release pairs. Some advantages of converting already_AddRefed to just returning raw nsCOMPtr/nsRefPtr are: 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. 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 temporary for you). You can just return whatever you like and the compiler will figure out if an addref is needed. There's a noticeable decrease in LoC when converting to use it. 3) We can get rid of a class, which simplifies the codebase. 4) If the callee already holds a strong reference in a local variable, it can just return that reference instead of a raw pointer. This saves an addref/release if the caller puts the result in an nsCOMPtr. Currently you could do this by returning an already_AddRefed, but that's annoying because of (1) above, so people don't always do so. 5) Callees that don't care about the risk of an extra addref/release can just return nsCOMPtr instead of raw pointers across the board. This avoids a potentially sec-critical use-after-free vulnerability that can occur if a function returns a raw pointer, then the caller passes it to a function that takes a raw pointer, then the second function does something to release it. This is very easy to do by mistake, because the bug just looks like "SomeFunction(GetThing());". Again, in theory you could already use already_AddRefed for this, but this change makes it painless. 6) In theory, the compiler can use RVO to elide the construction of the temporary altogether, although I'm not at all sure this is useful in practice. IMO, already_AddRefed is exactly the sort of use-case move constructors are meant to solve, and it's no longer needed once we have working move constructors for nsCOMPtr/nsRefPtr. I was planning to write up an informational post once bug 1015114 landed explaining the benefits and telling people how to convert everything. > 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? I've put some thought into this. Simple returns of already_AddRefed can definitely be converted without much effort, and it reduces LoC too. A bit more thought will be needed to get rid of the class entirely, I think. For one thing, we'd need a replacement for dont_AddRef(). Also, the move constructors themselves will need to be rewritten, because they use .forget() (a few friend declarations should do it). _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
