Set once pointer types and static analysis
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
Why isn’t the destructor a problem? Seems like we still need to protect the object if the SetOnceRefPtr itself might be destroyed. — Darin
On Oct 28, 2024, at 2:57 PM, Darin Adler <darin@apple.com> wrote:
Why isn’t the destructor a problem? Seems like we still need to protect the object if the SetOnceRefPtr itself might be destroyed.
We’d rely on transitive property. Since our rule / convention is that the caller of a member function keeps “this” object alive, the destructor of SetOnceRefPtr wouldn’t run while the member function is being called. - R. Niwa
I’m not sure it is worth the extra complexity personally.
On Oct 28, 2024, at 11:48 AM, Ryosuke Niwa via webkit-dev <webkit-dev@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@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
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@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@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
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@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@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@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
Instead of introducing a new smart pointer type like SetOnceRefPtr, we’re going to use `const RefPtr`, `const Ref`, `const unique_ptr`, and `const UniqueRef`. `const Ref` and `const UniqueRef` are used for values that are initialized in the constructor and never changed. `const RefPtr` and `const unique_ptr` are used for lazily initialized values. We introduce a `initializeOnce(ptr, v)` function which takes `const RefPtr` or `const unique_ptr` ptr and sets it to v. The static analyzer will be updated to recognize this pattern. https://bugs.webkit.org/show_bug.cgi?id=283038 https://github.com/llvm/llvm-project/pull/115594 - R. Niwa
On Oct 29, 2024, at 2:49 AM, youenn fablet <youennf@gmail.com> wrote:
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@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@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@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
Hi That sounds great. Can we also do the same for const RetainPtr ? Thanks
On 13 Nov 2024, at 4:49 PM, Ryosuke Niwa via webkit-dev <webkit-dev@lists.webkit.org> wrote:
Instead of introducing a new smart pointer type like SetOnceRefPtr, we’re going to use `const RefPtr`, `const Ref`, `const unique_ptr`, and `const UniqueRef`. `const Ref` and `const UniqueRef` are used for values that are initialized in the constructor and never changed. `const RefPtr` and `const unique_ptr` are used for lazily initialized values. We introduce a `initializeOnce(ptr, v)` function which takes `const RefPtr` or `const unique_ptr` ptr and sets it to v. The static analyzer will be updated to recognize this pattern.
https://bugs.webkit.org/show_bug.cgi?id=283038 https://github.com/llvm/llvm-project/pull/115594
- R. Niwa
On Oct 29, 2024, at 2:49 AM, youenn fablet <youennf@gmail.com> wrote:
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@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@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@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
RetainPtr isn’t supported by static analyzer at the moment.
On Nov 13, 2024, at 4:28 AM, Jean-Yves Avenard <jean-yves.avenard@apple.com> wrote:
Hi
That sounds great.
Can we also do the same for const RetainPtr ?
Thanks
On 13 Nov 2024, at 4:49 PM, Ryosuke Niwa via webkit-dev <webkit-dev@lists.webkit.org> wrote:
Instead of introducing a new smart pointer type like SetOnceRefPtr, we’re going to use `const RefPtr`, `const Ref`, `const unique_ptr`, and `const UniqueRef`. `const Ref` and `const UniqueRef` are used for values that are initialized in the constructor and never changed. `const RefPtr` and `const unique_ptr` are used for lazily initialized values. We introduce a `initializeOnce(ptr, v)` function which takes `const RefPtr` or `const unique_ptr` ptr and sets it to v. The static analyzer will be updated to recognize this pattern.
https://bugs.webkit.org/show_bug.cgi?id=283038 https://github.com/llvm/llvm-project/pull/115594
- R. Niwa
On Oct 29, 2024, at 2:49 AM, youenn fablet <youennf@gmail.com> wrote:
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@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@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@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
We’re naming the function `lazyInitialize` instead.
On Nov 12, 2024, at 9:49 PM, Ryosuke Niwa via webkit-dev <webkit-dev@lists.webkit.org> wrote:
Instead of introducing a new smart pointer type like SetOnceRefPtr, we’re going to use `const RefPtr`, `const Ref`, `const unique_ptr`, and `const UniqueRef`. `const Ref` and `const UniqueRef` are used for values that are initialized in the constructor and never changed. `const RefPtr` and `const unique_ptr` are used for lazily initialized values. We introduce a `initializeOnce(ptr, v)` function which takes `const RefPtr` or `const unique_ptr` ptr and sets it to v. The static analyzer will be updated to recognize this pattern.
https://bugs.webkit.org/show_bug.cgi?id=283038 https://github.com/llvm/llvm-project/pull/115594
- R. Niwa
On Oct 29, 2024, at 2:49 AM, youenn fablet <youennf@gmail.com> wrote:
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@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@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@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
participants (5)
-
Chris Dumez
-
Darin Adler
-
Jean-Yves Avenard
-
Ryosuke Niwa
-
youenn fablet