Smart Pointers Usage Guidelines
Hi all, It has been a while since I last announced the plan to adopt smart pointers using clang static analyzer: https://lists.webkit.org/pipermail/webkit-dev/2020-September/031386.html Here are some updates. 1. We’ve made a progress in implementing all the rules including rules for local variables in clang static analyzer: https://github.com/WebKit/WebKit/wiki/Smart-Pointer-Usage-Guidelines#rules-f... 2. We also have a new kind of smart pointers: CheckedRef <https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/CheckedRef.h> / CheckedPtr <https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/CheckedPtr.h>. These behave like Ref and RefPtr in that they increment & decrement a counter in an object but unlike them don’t extend the lifetime of the object. Instead, the destructor of the base object release asserts that there are no live CheckedRef / CheckedPtr left. I added a new section in the guide describing when to use each smart pointer type: https://github.com/WebKit/WebKit/wiki/Smart-Pointer-Usage-Guidelines#when-to... 3. I wanted to describe what applying these smart pointer rules mean. A lot of code changes needed for this work involves creating Ref / RefPtr / CheckedRef / CheckedPtr in stack: https://commits.webkit.org/267082@main One subtle thing is that even when a member variable is already Ref / RefPtr / CheckedRef / CheckedPtr, we must create another one in stack as seen here: https://commits.webkit.org/267108@main This is because these member variables can be cleared during the course of invoking a non-trivial function; or put it another way, it’s not immediately obvious from the code inspection that the object pointed to stays alive over the course of a non-trivial function call. - R. Niwa
On Aug 21, 2023, at 4:25 PM, Ryosuke Niwa via webkit-dev <webkit-dev@lists.webkit.org> wrote:
Hi all,
It has been a while since I last announced the plan to adopt smart pointers using clang static analyzer: https://lists.webkit.org/pipermail/webkit-dev/2020-September/031386.html
Here are some updates.
1. We’ve made a progress in implementing all the rules including rules for local variables in clang static analyzer: https://github.com/WebKit/WebKit/wiki/Smart-Pointer-Usage-Guidelines#rules-f...
2. We also have a new kind of smart pointers: CheckedRef <https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/CheckedRef.h> / CheckedPtr <https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/CheckedPtr.h>. These behave like Ref and RefPtr in that they increment & decrement a counter in an object but unlike them don’t extend the lifetime of the object. Instead, the destructor of the base object release asserts that there are no live CheckedRef / CheckedPtr left.
I added a new section in the guide describing when to use each smart pointer type: https://github.com/WebKit/WebKit/wiki/Smart-Pointer-Usage-Guidelines#when-to...
3. I wanted to describe what applying these smart pointer rules mean. A lot of code changes needed for this work involves creating Ref / RefPtr / CheckedRef / CheckedPtr in stack: https://commits.webkit.org/267082@main
One subtle thing is that even when a member variable is already Ref / RefPtr / CheckedRef / CheckedPtr, we must create another one in stack as seen here: https://commits.webkit.org/267108@main
(I asked rniwa to send this mail because this patch surprised me, so I hope now we can chat about it). The scope of this rule, and the … lack of elegance … at so many callsites worries me a bit. If it’s possible to automate enforcement, that might help with part of the problem, but it’s also just really not very pretty, and I wonder if someone can come up with some clever alternative solution before we go too far down this path (not me!). Alternatively, it’s possible other people OK with this syntax/requirement and I should just get over it. What do you all think?
This is because these member variables can be cleared during the course of invoking a non-trivial function; or put it another way, it’s not immediately obvious from the code inspection that the object pointed to stays alive over the course of a non-trivial function call.
- R. Niwa
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
On Aug 21, 2023, at 4:31 PM, Tim Horton via webkit-dev <webkit-dev@lists.webkit.org> wrote:
One subtle thing is that even when a member variable is already Ref / RefPtr / CheckedRef / CheckedPtr, we must create another one in stack as seen here: https://commits.webkit.org/267108@main
(I asked rniwa to send this mail because this patch surprised me, so I hope now we can chat about it).
The scope of this rule, and the … lack of elegance … at so many callsites worries me a bit. If it’s possible to automate enforcement, that might help with part of the problem, but it’s also just really not very pretty, and I wonder if someone can come up with some clever alternative solution before we go too far down this path (not me!).
Alternatively, it’s possible other people OK with this syntax/requirement and I should just get over it. What do you all think?
I hope we can make this better by using a getter function that returns a CheckedPtr so instead of writing CheckedPtr { _page }, we would write page(). — Darin
On Aug 21, 2023, at 4:34 PM, Darin Adler <darin@apple.com> wrote:
On Aug 21, 2023, at 4:31 PM, Tim Horton via webkit-dev <webkit-dev@lists.webkit.org> wrote:
One subtle thing is that even when a member variable is already Ref / RefPtr / CheckedRef / CheckedPtr, we must create another one in stack as seen here: https://commits.webkit.org/267108@main
(I asked rniwa to send this mail because this patch surprised me, so I hope now we can chat about it).
The scope of this rule, and the … lack of elegance … at so many callsites worries me a bit. If it’s possible to automate enforcement, that might help with part of the problem, but it’s also just really not very pretty, and I wonder if someone can come up with some clever alternative solution before we go too far down this path (not me!).
Alternatively, it’s possible other people OK with this syntax/requirement and I should just get over it. What do you all think?
I hope we can make this better by using a getter function that returns a CheckedPtr so instead of writing CheckedPtr { _page }, we would write page().
That, for example, seems wildly preferable.
— Darin
On Aug 21, 2023, at 4:34 PM, Darin Adler <darin@apple.com> wrote:
On Aug 21, 2023, at 4:31 PM, Tim Horton via webkit-dev <webkit-dev@lists.webkit.org> wrote:
One subtle thing is that even when a member variable is already Ref / RefPtr / CheckedRef / CheckedPtr, we must create another one in stack as seen here: https://commits.webkit.org/267108@main
(I asked rniwa to send this mail because this patch surprised me, so I hope now we can chat about it).
The scope of this rule, and the … lack of elegance … at so many callsites worries me a bit. If it’s possible to automate enforcement, that might help with part of the problem, but it’s also just really not very pretty, and I wonder if someone can come up with some clever alternative solution before we go too far down this path (not me!).
Alternatively, it’s possible other people OK with this syntax/requirement and I should just get over it. What do you all think?
I hope we can make this better by using a getter function that returns a CheckedPtr so instead of writing CheckedPtr { _page }, we would write page().
One drawback making a member function return CheckedPtr is that then code like this: `page()->document()->foo()` would cause a ref churn. Maybe we don’t care about such ref churns? But then a simpler rule to deploy will be that every function must return CheckedRef/CheckedPtr/Ref/RefPtr. Alternatively, we could add a new member function which returns CheckedPtr like `pageChecked()`. - R. Niwa
On Aug 21, 2023, at 4:39 PM, Ryosuke Niwa <rniwa@apple.com> wrote:
On Aug 21, 2023, at 4:34 PM, Darin Adler <darin@apple.com> wrote:
On Aug 21, 2023, at 4:31 PM, Tim Horton via webkit-dev <webkit-dev@lists.webkit.org> wrote:
One subtle thing is that even when a member variable is already Ref / RefPtr / CheckedRef / CheckedPtr, we must create another one in stack as seen here: https://commits.webkit.org/267108@main
(I asked rniwa to send this mail because this patch surprised me, so I hope now we can chat about it).
The scope of this rule, and the … lack of elegance … at so many callsites worries me a bit. If it’s possible to automate enforcement, that might help with part of the problem, but it’s also just really not very pretty, and I wonder if someone can come up with some clever alternative solution before we go too far down this path (not me!).
Alternatively, it’s possible other people OK with this syntax/requirement and I should just get over it. What do you all think?
I hope we can make this better by using a getter function that returns a CheckedPtr so instead of writing CheckedPtr { _page }, we would write page().
One drawback making a member function return CheckedPtr is that then code like this: `page()->document()->foo()` would cause a ref churn.
Maybe we don’t care about such ref churns?
How can we tell!
But then a simpler rule to deploy will be that every function must return CheckedRef/CheckedPtr/Ref/RefPtr.
+1 to that rule instead of the one in Wenson’s patch.
Alternatively, we could add a new member function which returns CheckedPtr like `pageChecked()`.
Would rather Darin’s plan. I don’t want to have to think about CheckedPtr every 5 seconds.
- R. Niwa
On Aug 21, 2023, at 4:41 PM, Darin Adler <darin@apple.com> wrote:
On Aug 21, 2023, at 4:39 PM, Ryosuke Niwa <rniwa@apple.com> wrote:
Alternatively, we could add a new member function which returns CheckedPtr like `pageChecked()`.
Yes, I think that would be a good approach that would complement the static checker.
Okay, let’s go with this solution since others have expressed concerns for ref churns in the past. - R. Niwa
participants (3)
-
Darin Adler
-
Ryosuke Niwa
-
Tim Horton