Re: [webkit-dev] Smart Pointers Usage Guidelines

2023-08-21 Thread Ryosuke Niwa via webkit-dev 

> On Aug 21, 2023, at 4:51 PM, Ryosuke Niwa via webkit-dev 
>  wrote:
> 
>> On Aug 21, 2023, at 4:50 PM, Tim Horton  wrote:
>> 
>>> On Aug 21, 2023, at 4:42 PM, Ryosuke Niwa  wrote:
>>> 
 On Aug 21, 2023, at 4:41 PM, Darin Adler  wrote:
 
> On Aug 21, 2023, at 4:39 PM, Ryosuke Niwa  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.
>> 
>> So, to be clear, this example:
>> 
>>> page()->document()->foo()
>> 
>> 
>> would become:
>> 
>>> page()->documentChecked()->foo()?
>> 
>> (only `checked` for the deepest getter before the complex call)?
> 
> Yes.

Now that I’m thinking about this more, we should probably call this 
checkedDocument() to be aligned with protectedX pattern.

- R. Niwa

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Smart Pointers Usage Guidelines

2023-08-21 Thread Ryosuke Niwa via webkit-dev 


> On Aug 21, 2023, at 4:50 PM, Tim Horton  wrote:
> 
> 
> 
>> On Aug 21, 2023, at 4:42 PM, Ryosuke Niwa  wrote:
>> 
>> 
>> 
>>> On Aug 21, 2023, at 4:41 PM, Darin Adler  wrote:
>>> 
 On Aug 21, 2023, at 4:39 PM, Ryosuke Niwa  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.
> 
> So, to be clear, this example:
> 
>> page()->document()->foo()
> 
> 
> would become:
> 
>> page()->documentChecked()->foo()?
> 
> (only `checked` for the deepest getter before the complex call)?

Yes.

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Smart Pointers Usage Guidelines

2023-08-21 Thread Ryosuke Niwa via webkit-dev 


> On Aug 21, 2023, at 4:41 PM, Darin Adler  wrote:
> 
>> On Aug 21, 2023, at 4:39 PM, Ryosuke Niwa  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

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Smart Pointers Usage Guidelines

2023-08-21 Thread Darin Adler via webkit-dev 
> On Aug 21, 2023, at 4:39 PM, Ryosuke Niwa  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.

— Darin___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Smart Pointers Usage Guidelines

2023-08-21 Thread Tim Horton via webkit-dev 


> On Aug 21, 2023, at 4:39 PM, Ryosuke Niwa  wrote:
> 
> 
>> On Aug 21, 2023, at 4:34 PM, Darin Adler  wrote:
>> 
>> 
>>> On Aug 21, 2023, at 4:31 PM, Tim Horton via webkit-dev 
>>>  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
> 

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Smart Pointers Usage Guidelines

2023-08-21 Thread Ryosuke Niwa via webkit-dev 

> On Aug 21, 2023, at 4:34 PM, Darin Adler  wrote:
> 
> 
>> On Aug 21, 2023, at 4:31 PM, Tim Horton via webkit-dev 
>>  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

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Smart Pointers Usage Guidelines

2023-08-21 Thread Tim Horton via webkit-dev 


> On Aug 21, 2023, at 4:34 PM, Darin Adler  wrote:
> 
> 
>> On Aug 21, 2023, at 4:31 PM, Tim Horton via webkit-dev 
>>  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

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Smart Pointers Usage Guidelines

2023-08-21 Thread Darin Adler via webkit-dev 

> On Aug 21, 2023, at 4:31 PM, Tim Horton via webkit-dev 
>  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___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Smart Pointers Usage Guidelines

2023-08-21 Thread Tim Horton via webkit-dev 

> On Aug 21, 2023, at 4:25 PM, Ryosuke Niwa via webkit-dev 
>  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-for-using-ref-counted-objects
> 
> 
> 2. We also have a new kind of smart pointers: CheckedRef 
>  / 
> CheckedPtr 
> . 
> 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-use-which-smart-pointer
> 
> 
> 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

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

[webkit-dev] Smart Pointers Usage Guidelines

2023-08-21 Thread Ryosuke Niwa via webkit-dev 
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-for-using-ref-counted-objects


2. We also have a new kind of smart pointers: CheckedRef 
 / 
CheckedPtr 
. 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-use-which-smart-pointer


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

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev