Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit
> On Jan 30, 2023, at 6:29 PM, Fujii Hironori via webkit-dev > wrote: > > On Tue, Jan 31, 2023 at 8:28 AM Ryosuke Niwa via webkit-dev > wrote: >> >> I’ve posted https://github.com/WebKit/WebKit/wiki/Smart-Pointer-Usage-Rules >> >> > Very nice. Can I add the following exception for > webkit.UncountedLambdaCapturesChecker rule? > > https://lists.webkit.org/pipermail/webkit-dev/2020-September/031405.html > > > We probably also need to figure out a way to exempt all lambda functions > > that never get stored anywhere. We have a bunch of helper functions like > > WTF::map which just calls lambdas on each item while iterating over an > > array, etc... and there is no need to create a separate Ref / RefPtr in > > those cases since lambdas are never stored and re-used later. Done. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit
On Tue, Jan 31, 2023 at 8:28 AM Ryosuke Niwa via webkit-dev < webkit-dev@lists.webkit.org> wrote: > > I’ve posted > https://github.com/WebKit/WebKit/wiki/Smart-Pointer-Usage-Rules > > Very nice. Can I add the following exception for webkit.UncountedLambdaCapturesChecker rule? https://lists.webkit.org/pipermail/webkit-dev/2020-September/031405.html > We probably also need to figure out a way to exempt all lambda functions > that never get stored anywhere. We have a bunch of helper functions like > WTF::map which just calls lambdas on each item while iterating over an > array, etc... and there is no need to create a separate Ref / RefPtr in > those cases since lambdas are never stored and re-used later. ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit
> On Jan 30, 2023, at 12:11 PM, Ryosuke Niwa via webkit-dev > wrote: > >> On Jan 30, 2023, at 10:47 AM, Alexey Proskuryakov wrote: >> >> Reviving this old thread, reading it again made me wish for two things: >> >> - a wiki document that describes what we are trying to do, not just a thread >> which patches the proposal with clarifications; > > Yeah, let me go make one. I’ve posted https://github.com/WebKit/WebKit/wiki/Smart-Pointer-Usage-Rules >> - a discussion of why we can postpone figuring out what to do with >> containers (like Vector or HashMap> RenderFragmentContainer*>). > > This was probably an oversight on my part. The intention is to make member > variables / local variables of container type should also be using smart > pointers in its type: e.g. Vector> instead of Vector. > WeakHashMap> instead of > HashMap. I’ll try to clarify this in > the new doc I make. Fixed in the new wiki documentation. - R. Niwa >>> 23 сент. 2020 г., в 1:54 PM, Jan Korous написал(а): >>> >>> Hi all, >>> >>> I am an engineer at Security Tools team at Apple responsible for the >>> tooling support for this effort. >>> On Sep 23, 2020, at 12:34 PM, Darin Adler wrote: > On Sep 23, 2020, at 12:18 PM, Ryosuke Niwa wrote: > > There are quite a few cases where data members are references but then > those can also be replaced by a simple member function which retrieves > the value of the smart pointer member variable and returns a reference. I think this should be an explicit recommendation in the project of refactoring to follow these rules. > For now, a trivial function is defined as a member function defined in > the class declaration whose definition simply returns a member variable > (the result of get() or a copy if the member variable is a smart pointer). That seems like a rule that’s too narrow. I would not want a function to become non-trivial just because I moved it from being inline within the class definition to an inline below the class definition in the same header. This rule worries me a lot right now; it seems like it could result in an explosion of local variable copies of arguments. > We probably also need to figure out a way to exempt all lambda functions > that never get stored anywhere. We have a bunch of helper functions like > WTF::map which just calls lambdas on each item while iterating over an > array, etc... and there is no need to create a separate Ref / RefPtr in > those cases since lambdas are never stored and re-used later. Does seem important. I am pretty sure I have seen this concept in other languages. We often try to use const Function& for one type of lambda argument and Function&& for the other type, but that’s far from complete. >>> >>> Re: lambda captures - I think we have two ideas here. >>> >>> 1. Allow WeakPtr captures. >>> This makes sense to do but it implies we have to add the notion of >>> ownership to the rules. The thing is that WeakPtr is safe on its own (and >>> technically reference-counted) but it can’t be used as a safety measure in >>> the way of RefPtr or Ref since it doesn’t own the memory (not even in a >>> shared manner). >>> >>> 2. Allow raw pointer/reference captures. >>> This makes sense given you use generic algorithms in the codebase. I will >>> implement a new version of the checker - currently it is still based on >>> simple AST analysis and for this kind of reasoning we’ll need to use >>> symbolic execution in Clang Static Analyzer. >>> >>> Thanks, >>> >>> Jan >>> — Darin >>> >>> ___ >>> 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 mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit
> On Jan 30, 2023, at 10:47 AM, Alexey Proskuryakov wrote: > > Reviving this old thread, reading it again made me wish for two things: > > - a wiki document that describes what we are trying to do, not just a thread > which patches the proposal with clarifications; Yeah, let me go make one. > - a discussion of why we can postpone figuring out what to do with containers > (like Vector or HashMap). This was probably an oversight on my part. The intention is to make member variables / local variables of container type should also be using smart pointers in its type: e.g. Vector> instead of Vector. WeakHashMap> instead of HashMap. I’ll try to clarify this in the new doc I make. - R. Niwa >> 23 сент. 2020 г., в 1:54 PM, Jan Korous написал(а): >> >> Hi all, >> >> I am an engineer at Security Tools team at Apple responsible for the tooling >> support for this effort. >> >>> On Sep 23, 2020, at 12:34 PM, Darin Adler wrote: >>> On Sep 23, 2020, at 12:18 PM, Ryosuke Niwa wrote: There are quite a few cases where data members are references but then those can also be replaced by a simple member function which retrieves the value of the smart pointer member variable and returns a reference. >>> >>> I think this should be an explicit recommendation in the project of >>> refactoring to follow these rules. >>> For now, a trivial function is defined as a member function defined in the class declaration whose definition simply returns a member variable (the result of get() or a copy if the member variable is a smart pointer). >>> >>> That seems like a rule that’s too narrow. I would not want a function to >>> become non-trivial just because I moved it from being inline within the >>> class definition to an inline below the class definition in the same header. >>> >>> This rule worries me a lot right now; it seems like it could result in an >>> explosion of local variable copies of arguments. >>> We probably also need to figure out a way to exempt all lambda functions that never get stored anywhere. We have a bunch of helper functions like WTF::map which just calls lambdas on each item while iterating over an array, etc... and there is no need to create a separate Ref / RefPtr in those cases since lambdas are never stored and re-used later. >>> >>> Does seem important. I am pretty sure I have seen this concept in other >>> languages. We often try to use const Function& for one type of lambda >>> argument and Function&& for the other type, but that’s far from complete. >> >> Re: lambda captures - I think we have two ideas here. >> >> 1. Allow WeakPtr captures. >> This makes sense to do but it implies we have to add the notion of ownership >> to the rules. The thing is that WeakPtr is safe on its own (and technically >> reference-counted) but it can’t be used as a safety measure in the way of >> RefPtr or Ref since it doesn’t own the memory (not even in a shared manner). >> >> 2. Allow raw pointer/reference captures. >> This makes sense given you use generic algorithms in the codebase. I will >> implement a new version of the checker - currently it is still based on >> simple AST analysis and for this kind of reasoning we’ll need to use >> symbolic execution in Clang Static Analyzer. >> >> Thanks, >> >> Jan >> >>> — Darin >> >> ___ >> 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
Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit
I am hoping this ends up being practical. In the CSS code I have recently worked on there are many short-lived references, and in some refactoring I was not able to change them all to smart pointers without a measurable performance degradation in Speedometer, so the code might need redesign to use different idioms. — Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit
Hi, Reviving this old thread, reading it again made me wish for two things: - a wiki document that describes what we are trying to do, not just a thread which patches the proposal with clarifications; - a discussion of why we can postpone figuring out what to do with containers (like Vector or HashMap). - Alexey > 23 сент. 2020 г., в 1:54 PM, Jan Korous написал(а): > > Hi all, > > I am an engineer at Security Tools team at Apple responsible for the tooling > support for this effort. > >> On Sep 23, 2020, at 12:34 PM, Darin Adler wrote: >> >>> On Sep 23, 2020, at 12:18 PM, Ryosuke Niwa wrote: >>> >>> There are quite a few cases where data members are references but then >>> those can also be replaced by a simple member function which retrieves the >>> value of the smart pointer member variable and returns a reference. >> >> I think this should be an explicit recommendation in the project of >> refactoring to follow these rules. >> >>> For now, a trivial function is defined as a member function defined in the >>> class declaration whose definition simply returns a member variable (the >>> result of get() or a copy if the member variable is a smart pointer). >> >> That seems like a rule that’s too narrow. I would not want a function to >> become non-trivial just because I moved it from being inline within the >> class definition to an inline below the class definition in the same header. >> >> This rule worries me a lot right now; it seems like it could result in an >> explosion of local variable copies of arguments. >> >>> We probably also need to figure out a way to exempt all lambda functions >>> that never get stored anywhere. We have a bunch of helper functions like >>> WTF::map which just calls lambdas on each item while iterating over an >>> array, etc... and there is no need to create a separate Ref / RefPtr in >>> those cases since lambdas are never stored and re-used later. >> >> Does seem important. I am pretty sure I have seen this concept in other >> languages. We often try to use const Function& for one type of lambda >> argument and Function&& for the other type, but that’s far from complete. > > Re: lambda captures - I think we have two ideas here. > > 1. Allow WeakPtr captures. > This makes sense to do but it implies we have to add the notion of ownership > to the rules. The thing is that WeakPtr is safe on its own (and technically > reference-counted) but it can’t be used as a safety measure in the way of > RefPtr or Ref since it doesn’t own the memory (not even in a shared manner). > > 2. Allow raw pointer/reference captures. > This makes sense given you use generic algorithms in the codebase. I will > implement a new version of the checker - currently it is still based on > simple AST analysis and for this kind of reasoning we’ll need to use symbolic > execution in Clang Static Analyzer. > > Thanks, > > Jan > >> — Darin > > ___ > 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
