[webkit-dev] Set once pointer types and static analysis

[youenn fablet]

FWIW, there are many classes with Ref<> members that are initialized in the
constructor and are never intended to be changed.
I was hoping we would cover this case at least. The RefPtr lazy
initialization is a welcome addition as well.

I think this would be useful to improve readability:
- Too many protected in the same line of code makes code harder to read for
me.
- It is easier to reason about "stable" member variables. Having an
explicit type or some annotation instead of having to look at the whole
code is an improvement.

Le mar. 29 oct. 2024 à 03:10, Jean-Yves Avenard via webkit-dev <
webkit-dev at lists.webkit.org> a écrit :

> Hi
>
> It’s a very strong +1 for me ; I find the usage of
> foo->protectedBar()->method() or Ref { foo->bar() }->method()
> particularly unsightly (and points more to the inability of the static
> analyser to deal with some use case than the code being inherently unsafe)
>
> Having a way to reduce the unsightly pattern where we are 100% certain
> it’s not needed is a benefit.
> I assume also that the static analyser will not complain with `const`
> members either
>
> (For the sake of disclosure, I asked Ryosuke for that feature after
> discussing the need with Youenn)
>
> Jean-Yves
>
> On 29 Oct 2024, at 5:48 am, Ryosuke Niwa via webkit-dev <
> webkit-dev at lists.webkit.org> wrote:
>
> Hi all,
>
> In WebKit, it’s fairly common to write a member variable as RefPtr or
> std::unique_ptr that later gets lazily initialized to some value but never
> unset or assigned of a different value after that.
>
> e.g.
>
> class Foo {
>     Bar& bar() {
>         if (!m_bar)
>             m_bar = Bar::create();
>         return *m_bar;
>     }
>     Ref<Bar> protectedBar() { return bar(); }
>
>     RefPtr<Bar> m_bar;
> }
>
> Assuming there is no other code modifying m_bar, foo->bar()->method() is
> always safe to call even if method wasn’t a trivial function. Right now,
> static analyzer doesn’t recognize this pattern so we’d be forced to write
> code like this: foo->protectedBar()->method() where ensureProtectedBar is
> a wrapper around ensureBar which returns Ref<Bar>.
>
> A suggestion was made that static analyzer can recognize this patten.
> Specifically, if we introduced a new smart pointer types that only allow
> setting the value once, static analyzer can allow foo->bar()->method()and
> avoid ref-churn in some cases:
>
> e.g.
>
> class Foo {
>     Bar& bar() {
>         if (!m_bar)
>             m_bar = Bar::create();
>         return *m_bar;
>     }
>
>     SetOnceRefPtr<Bar> m_bar;
> }
>
> SetOnceRefPtr::operator=(T*) release asserts that m_ptr isn’t set, and
> doesn’t have a move constructor, operator=(nullptr_t), leakRef(),
> releaseNonNull(), etc… which can override the value of m_ptr after
> setting the value via operator= or in constructor.
>
> We could create various variants: SetOnceRef, SetOnceUniquePtr,
> SetOnceUniqueRef.
>
> Do you think this will be useful?
>
> - R. Niwa
>
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
>
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20241029/84a9f117/attachment.htm>