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 returns a reference into a raw pointer, which isn't > > possible with already_AddRefed. In other words, today, you can > > write: > > T* t = GetT() > > and know that as long as the code is following conventions > > correctly and using already_AddRefed when a reference is returned, > > this won't compile if an nsCOMPtr is required. If we change to > > returning nsCOMPtr instead of returning already_AddRefed, this > > won't be the case, and the code will end up having a dangling > > pointer to a potentially-deleted object. > > 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 potential use-after-free, and that definitely > has happened already. Reviewers need to make sure that refcounted > types aren't ever kept in raw pointers in local variables, unless > perhaps it's very clear from the code that nothing can possibly call > Release() (although it still makes me nervous).
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 function. For example, if I open content/base/src/Element.cpp, I see this pattern 5 times in the first 325 lines of the file. (Three nsIDocument*, one nsIContent*, one nsIPresShell*.) -David -- 𝄞 L. David Baron http://dbaron.org/ 𝄂 𝄢 Mozilla https://www.mozilla.org/ 𝄂 Before I built a wall I'd ask to know What I was walling in or walling out, And to whom I was like to give offense. - Robert Frost, Mending Wall (1914)
signature.asc
Description: Digital signature
_______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
