Re: [webkit-dev] Tightening up smart pointer usage rules
I like leakPtr. Personally, releasePtr() is so close to release() (which is safe) that I could easily overlook it in a code review and not realize that it hands off a raw pointer with a ref count on it. dave On Tue, Jun 29, 2010 at 10:40 AM, Darin Adler da...@apple.com wrote: On Jun 28, 2010, at 7:08 PM, Maciej Stachowiak wrote: - Yes, we should get rid of auto_ptr. - I can imagine a leakPtr function being more self-documenting than adoptPtr(...).releasePtr() when it appears in code, but on the other hand the greater convenience may lead to using it carelessly. On the third hand, it will definitely stand out if it appears in a patch. The main benefit of adoptPtr(...).releasePtr() is that the adoptPtr can stay with the new when refactoring, and the releasePtr can stay with the intentional leak. Any call site using releasePtr needs to be examined carefully to see it does not leak. That’s different from functions like release that do not pose that sort of risk, but similar to releaseRef, which does. If we think the word leak is key to clarity here, renaming releasePtr to leakPtr would be one option. I don’t think we want a single function combining the adoptPtr and releasePtr behavior. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Tightening up smart pointer usage rules
I like leakPtr. Personally, releasePtr() is so close to release() (which is safe) that I could easily overlook it in a code review and not realize that it hands off a raw pointer with a ref count on it. dave +1 Anders once argued against the name leakPtr, Well, then you might as well rename 'malloc' to 'leak'. The more I think about it, though, the more I think that malloc really should be named leak, at least from the WebKit project's perspective. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Tightening up smart pointer usage rules
Hi folks.
I’d like to use our smart pointers more consistently to decrease the chances of
memory leaks or other similar problems.
My proposal is that we have a rule that all calls to new are immediately
followed by putting the object into a smart pointer. The main types of smart
pointer that matter for these purposes are:
RefPtr
OwnPtr
OwnArrayPtr
Today, we put a new reference counted object into a RefPtr by calling adoptRef.
The code looks something like this:
PassRefPtrSharedObject createSharedObject()
{
return adoptRef(new SharedObject);
}
I propose we do the same thing with single ownership objects. It would look
like this:
PassOwnPtrSingleOwnerObject createSingleOwnerObject()
{
return adoptPtr(new SingleOwnerObject);
}
Then it would be a programming mistake to call new without immediately calling
adoptPtr or adoptRef. As part of this effort I plan to do the following:
1) Add a debugging mode where we assert at runtime if we ref, deref, or
destroy a RefCounted object before doing adoptRef. Tracked in
https://bugs.webkit.org/show_bug.cgi?id=27672.
2) Add an adoptPtr function that returns a PassOwnPtr, and a releasePtr
function that returns a raw pointer to the PassOwnPtr class.
3) Add a PassOwnArrayPtr with similar adoptArrayPtr and releaseArrayPtr
functions.
4) Add a strict mode to PassOwnPtr and OwnPtr which removes OwnPtr::set
entirely and removes the ability to construct a PassOwnPtr or OwnPtr from a raw
pointer directly, making it a compile time error to forget to use adoptPtr.
5) Once everything compiles with the strict mode, make the strict mode the
only mode.
6) Add validator rules that make invocation of the new operator legal
only inside adoptRef and adoptPtr function calls.
Code that used to say this:
OwnPtrOtherObject m_otherObject;
...
m_otherObject.set(new OtherObject);
Will now say this:
OwnPtrOtherObject m_otherObject;
...
m_otherObject = adoptPtr(new OtherObject);
And one thing that’s cool about that is it is quite natural for this to become:
OwnPtrOtherObject m_otherObject;
...
m_otherObject = OtherObject::create();
I thought I’d mention this to everyone on the list before getting doing
significantly more work on this. I think it’s going to work well. Any questions
or comments?
-- Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Tightening up smart pointer usage rules
6) Add validator rules that make invocation of the new operator legal only inside adoptRef and adoptPtr function calls. That is probably going to be a problem for Qt code on the WebKit API side. If we disable this rule for WebKitTools and WebKit I think we should be OK Kenneth ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Tightening up smart pointer usage rules
On Mon, Jun 28, 2010 at 1:43 PM, Kenneth Christiansen kenneth.christian...@openbossa.org wrote: 6) Add validator rules that make invocation of the new operator legal only inside adoptRef and adoptPtr function calls. That is probably going to be a problem for Qt code on the WebKit API side. If we disable this rule for WebKitTools and WebKit I think we should be OK We already have similar exceptions in the validator for code in the embedder's style. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Tightening up smart pointer usage rules
Sounds great. Let me know how I can help.
Do we need an exception for statics that we intend to leak at
shutdown? Maybe leakPtr(new Foo)?
Adam
On Mon, Jun 28, 2010 at 1:39 PM, Darin Adler da...@apple.com wrote:
Hi folks.
I’d like to use our smart pointers more consistently to decrease the chances
of memory leaks or other similar problems.
My proposal is that we have a rule that all calls to new are immediately
followed by putting the object into a smart pointer. The main types of smart
pointer that matter for these purposes are:
RefPtr
OwnPtr
OwnArrayPtr
Today, we put a new reference counted object into a RefPtr by calling
adoptRef. The code looks something like this:
PassRefPtrSharedObject createSharedObject()
{
return adoptRef(new SharedObject);
}
I propose we do the same thing with single ownership objects. It would look
like this:
PassOwnPtrSingleOwnerObject createSingleOwnerObject()
{
return adoptPtr(new SingleOwnerObject);
}
Then it would be a programming mistake to call new without immediately
calling adoptPtr or adoptRef. As part of this effort I plan to do the
following:
1) Add a debugging mode where we assert at runtime if we ref, deref, or
destroy a RefCounted object before doing adoptRef. Tracked in
https://bugs.webkit.org/show_bug.cgi?id=27672.
2) Add an adoptPtr function that returns a PassOwnPtr, and a releasePtr
function that returns a raw pointer to the PassOwnPtr class.
3) Add a PassOwnArrayPtr with similar adoptArrayPtr and releaseArrayPtr
functions.
4) Add a strict mode to PassOwnPtr and OwnPtr which removes OwnPtr::set
entirely and removes the ability to construct a PassOwnPtr or OwnPtr from a
raw pointer directly, making it a compile time error to forget to use
adoptPtr.
5) Once everything compiles with the strict mode, make the strict mode the
only mode.
6) Add validator rules that make invocation of the new operator legal
only inside adoptRef and adoptPtr function calls.
Code that used to say this:
OwnPtrOtherObject m_otherObject;
...
m_otherObject.set(new OtherObject);
Will now say this:
OwnPtrOtherObject m_otherObject;
...
m_otherObject = adoptPtr(new OtherObject);
And one thing that’s cool about that is it is quite natural for this to
become:
OwnPtrOtherObject m_otherObject;
...
m_otherObject = OtherObject::create();
I thought I’d mention this to everyone on the list before getting doing
significantly more work on this. I think it’s going to work well. Any
questions or comments?
-- Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Tightening up smart pointer usage rules
Is the plan also to banish the std::auto_ptr? It seems pointless and
confusing to allow both the OwnPtr and the auto_ptr.
antti (who wants PwnPtr too)
On Mon, Jun 28, 2010 at 11:39 PM, Darin Adler da...@apple.com wrote:
Hi folks.
I’d like to use our smart pointers more consistently to decrease the chances
of memory leaks or other similar problems.
My proposal is that we have a rule that all calls to new are immediately
followed by putting the object into a smart pointer. The main types of smart
pointer that matter for these purposes are:
RefPtr
OwnPtr
OwnArrayPtr
Today, we put a new reference counted object into a RefPtr by calling
adoptRef. The code looks something like this:
PassRefPtrSharedObject createSharedObject()
{
return adoptRef(new SharedObject);
}
I propose we do the same thing with single ownership objects. It would look
like this:
PassOwnPtrSingleOwnerObject createSingleOwnerObject()
{
return adoptPtr(new SingleOwnerObject);
}
Then it would be a programming mistake to call new without immediately
calling adoptPtr or adoptRef. As part of this effort I plan to do the
following:
1) Add a debugging mode where we assert at runtime if we ref, deref, or
destroy a RefCounted object before doing adoptRef. Tracked in
https://bugs.webkit.org/show_bug.cgi?id=27672.
2) Add an adoptPtr function that returns a PassOwnPtr, and a releasePtr
function that returns a raw pointer to the PassOwnPtr class.
3) Add a PassOwnArrayPtr with similar adoptArrayPtr and releaseArrayPtr
functions.
4) Add a strict mode to PassOwnPtr and OwnPtr which removes OwnPtr::set
entirely and removes the ability to construct a PassOwnPtr or OwnPtr from a
raw pointer directly, making it a compile time error to forget to use
adoptPtr.
5) Once everything compiles with the strict mode, make the strict mode the
only mode.
6) Add validator rules that make invocation of the new operator legal
only inside adoptRef and adoptPtr function calls.
Code that used to say this:
OwnPtrOtherObject m_otherObject;
...
m_otherObject.set(new OtherObject);
Will now say this:
OwnPtrOtherObject m_otherObject;
...
m_otherObject = adoptPtr(new OtherObject);
And one thing that’s cool about that is it is quite natural for this to
become:
OwnPtrOtherObject m_otherObject;
...
m_otherObject = OtherObject::create();
I thought I’d mention this to everyone on the list before getting doing
significantly more work on this. I think it’s going to work well. Any
questions or comments?
-- Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Tightening up smart pointer usage rules
On Jun 28, 2010, at 2:14 PM, Adam Barth wrote: Do we need an exception for statics that we intend to leak at shutdown? Maybe leakPtr(new Foo)? Maybe. My first thought for such things is that I’d prefer to write them as: adoptPtr(new Foo).releasePtr() At first that may look strange, but it has a huge benefit. It can be refactored to: Foo::create().releasePtr() That’s the main reason I don’t suggest having a leakPtr or dontAdoptPtr function. I have thought that leakPtr might be a good alternate name for releasePtr. In fact I have proposed renaming releaseRef to leakRef more than once. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Tightening up smart pointer usage rules
On Jun 28, 2010, at 2:55 PM, Antti Koivisto wrote: Is the plan also to banish the std::auto_ptr? It seems pointless and confusing to allow both the OwnPtr and the auto_ptr. Yes, that would be my preference. PassOwnPtr is intended as a replacement for std::auto_ptr. Before PassOwnPtr existed, we used auto_ptr, but now we should replace any use of auto_ptr with PassOwnPtr and OwnPtr. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Tightening up smart pointer usage rules
I like this idea. Addressing some of the side comments:
- Yes, we should get rid of auto_ptr.
- I can imagine a leakPtr function being more self-documenting than
adoptPtr(...).releasePtr() when it appears in code, but on the other hand the
greater convenience may lead to using it carelessly. On the third hand, it will
definitely stand out if it appears in a patch.
Regards,
Maciej
On Jun 28, 2010, at 1:39 PM, Darin Adler wrote:
Hi folks.
I’d like to use our smart pointers more consistently to decrease the chances
of memory leaks or other similar problems.
My proposal is that we have a rule that all calls to new are immediately
followed by putting the object into a smart pointer. The main types of smart
pointer that matter for these purposes are:
RefPtr
OwnPtr
OwnArrayPtr
Today, we put a new reference counted object into a RefPtr by calling
adoptRef. The code looks something like this:
PassRefPtrSharedObject createSharedObject()
{
return adoptRef(new SharedObject);
}
I propose we do the same thing with single ownership objects. It would look
like this:
PassOwnPtrSingleOwnerObject createSingleOwnerObject()
{
return adoptPtr(new SingleOwnerObject);
}
Then it would be a programming mistake to call new without immediately
calling adoptPtr or adoptRef. As part of this effort I plan to do the
following:
1) Add a debugging mode where we assert at runtime if we ref, deref, or
destroy a RefCounted object before doing adoptRef. Tracked in
https://bugs.webkit.org/show_bug.cgi?id=27672.
2) Add an adoptPtr function that returns a PassOwnPtr, and a releasePtr
function that returns a raw pointer to the PassOwnPtr class.
3) Add a PassOwnArrayPtr with similar adoptArrayPtr and releaseArrayPtr
functions.
4) Add a strict mode to PassOwnPtr and OwnPtr which removes OwnPtr::set
entirely and removes the ability to construct a PassOwnPtr or OwnPtr from a
raw pointer directly, making it a compile time error to forget to use
adoptPtr.
5) Once everything compiles with the strict mode, make the strict mode the
only mode.
6) Add validator rules that make invocation of the new operator legal
only inside adoptRef and adoptPtr function calls.
Code that used to say this:
OwnPtrOtherObject m_otherObject;
...
m_otherObject.set(new OtherObject);
Will now say this:
OwnPtrOtherObject m_otherObject;
...
m_otherObject = adoptPtr(new OtherObject);
And one thing that’s cool about that is it is quite natural for this to
become:
OwnPtrOtherObject m_otherObject;
...
m_otherObject = OtherObject::create();
I thought I’d mention this to everyone on the list before getting doing
significantly more work on this. I think it’s going to work well. Any
questions or comments?
-- Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
