Bug 227164

Summary: Add CheckedRef
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Web Template FrameworkAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, benjamin, cdumez, cmarcelo, darin, ews-watchlist, ggaren, gyuyoung.kim, Hironori.Fujii, koivisto, ryuan.choi, sergio, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=226158
Attachments:
Description Flags
Patch
none
Updated for ToT
none
Patch
none
Patch none

Description Ryosuke Niwa 2021-06-18 03:22:05 PDT
Add CheckedRef, which is a reference version of CheckedPtr.
Comment 1 Ryosuke Niwa 2021-06-19 14:08:00 PDT
Created attachment 431805 [details]
Patch
Comment 2 Ryosuke Niwa 2021-06-20 00:30:05 PDT
Created attachment 431815 [details]
Updated for ToT
Comment 3 Fujii Hironori 2021-06-21 20:45:03 PDT
Comment on attachment 431815 [details]
Updated for ToT

View in context: https://bugs.webkit.org/attachment.cgi?id=431815&action=review

Do you need to fix style checker errors?

> Source/WTF/wtf/CheckedPtr.h:67
> +        : CheckedPtr(other.m_ptr)

I think you needd to unwrap here.
PtrTraits::unwrap(other.m_ptr)
Or, more simply other.get().

> Source/WTF/wtf/CheckedPtr.h:85
> +        ASSERT(m_ptr);

CheckedPtr can accept nullptr. Why do you check ASSERT(m_ptr)?
And, the type of m_ptr is PtrTraits::StorageType. Do  you need to unwrap? ASSERT(PtrTraits::unwrap(m_ptr))
Comment 4 Fujii Hironori 2021-06-21 20:55:46 PDT
Comment on attachment 431815 [details]
Updated for ToT

View in context: https://bugs.webkit.org/attachment.cgi?id=431815&action=review

> Source/WTF/wtf/CheckedRef.h:47
> +        : m_ptr(&object)

Can you store a raw pointer directly to PtrTraits::StorageType? Do you need to use PtrTraits::exchange?

> Source/WTF/wtf/CheckedRef.h:60
> +        : m_ptr { other.m_ptr }

Can you store a OtherPtrTraits::StorageType directly to PtrTraits::StorageType?
Comment 5 Fujii Hironori 2021-06-21 21:10:51 PDT
Comment on attachment 431815 [details]
Updated for ToT

View in context: https://bugs.webkit.org/attachment.cgi?id=431815&action=review

> Source/WTF/wtf/CheckedRef.h:171
> +template<typename T, typename PtrTraits = RawPtrTraits<T>> inline CheckedRef<T, PtrTraits> makeCheckedRef(T& reference)

You removed makeCheckedPtr. Why do you add makeCheckedRef?
Comment 6 Radar WebKit Bug Importer 2021-06-25 03:23:27 PDT
<rdar://problem/79770147>
Comment 7 Ryosuke Niwa 2021-07-28 22:59:00 PDT
Comment on attachment 431815 [details]
Updated for ToT

View in context: https://bugs.webkit.org/attachment.cgi?id=431815&action=review

>> Source/WTF/wtf/CheckedPtr.h:67
>> +        : CheckedPtr(other.m_ptr)
> 
> I think you needd to unwrap here.
> PtrTraits::unwrap(other.m_ptr)
> Or, more simply other.get().

Good point. Fixed.

>> Source/WTF/wtf/CheckedPtr.h:85
>> +        ASSERT(m_ptr);
> 
> CheckedPtr can accept nullptr. Why do you check ASSERT(m_ptr)?
> And, the type of m_ptr is PtrTraits::StorageType. Do  you need to unwrap? ASSERT(PtrTraits::unwrap(m_ptr))

Actually, we should just assert get().
We need to assert this because it's possible for someone to use-after-move on other, which case m_ptr will be nullptr.

>> Source/WTF/wtf/CheckedRef.h:60
>> +        : m_ptr { other.m_ptr }
> 
> Can you store a OtherPtrTraits::StorageType directly to PtrTraits::StorageType?

Good point. We should call other.get() here as well.

>> Source/WTF/wtf/CheckedRef.h:171
>> +template<typename T, typename PtrTraits = RawPtrTraits<T>> inline CheckedRef<T, PtrTraits> makeCheckedRef(T& reference)
> 
> You removed makeCheckedPtr. Why do you add makeCheckedRef?

Oops, forgot to remove this. Removed.
Comment 8 Ryosuke Niwa 2021-07-29 02:20:45 PDT
Created attachment 434505 [details]
Patch
Comment 9 Simon Fraser (smfr) 2021-07-29 08:56:11 PDT
Comment on attachment 434505 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434505&action=review

> Source/WTF/wtf/CheckedRef.h:33
> +

Please add a comment here summarizing what this class is for. CheckedPtr.h needs one too.
Comment 10 Darin Adler 2021-07-29 13:40:22 PDT
Comment on attachment 434505 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434505&action=review

> Source/WTF/wtf/CheckedPtr.h:82
> +    CheckedPtr(const CheckedRef<T, PtrTraits>&& other)

The const here seems wrong. If this is an rvalue reference it should not have const. Not sure why there are no tests that fail because of this.
Comment 11 Ryosuke Niwa 2021-07-29 14:55:47 PDT
(In reply to Simon Fraser (smfr) from comment #9)
> Comment on attachment 434505 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434505&action=review
> 
> > Source/WTF/wtf/CheckedRef.h:33
> > +
> 
> Please add a comment here summarizing what this class is for. CheckedPtr.h
> needs one too.

Why? When did it become customary to add such a comment? I don't recall having that kind of discussion on webkit-dev.
Comment 12 Ryosuke Niwa 2021-07-29 15:05:24 PDT
(In reply to Darin Adler from comment #10)
> Comment on attachment 434505 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434505&action=review
> 
> > Source/WTF/wtf/CheckedPtr.h:82
> > +    CheckedPtr(const CheckedRef<T, PtrTraits>&& other)
> 
> The const here seems wrong. If this is an rvalue reference it should not
> have const. Not sure why there are no tests that fail because of this.

Oh, that must be because we have also have:

    template<typename OtherType, typename OtherPtrTraits> CheckedPtr(CheckedRef<OtherType, OtherPtrTraits>&& other)
        : m_ptr { other.releasePtr() }
    {
        ASSERT(get());
    }

I guess we can remove this const version. No need to have two of the same function.
Comment 13 Darin Adler 2021-07-29 15:09:05 PDT
Sometimes we need a non-template constructor because of how that affects overload resolution; something about non-template constructors working with implicit conversions in a way that a template one does not. Presumably we could construct a test for those cases. For now, fine to land without it, but it would be good to be consistent across all our smart pointer templates.
Comment 14 Ryosuke Niwa 2021-07-29 15:15:22 PDT
(In reply to Darin Adler from comment #13)
> Sometimes we need a non-template constructor because of how that affects
> overload resolution; something about non-template constructors working with
> implicit conversions in a way that a template one does not. Presumably we
> could construct a test for those cases. For now, fine to land without it,
> but it would be good to be consistent across all our smart pointer templates.

Oh, uncanny. I guess I do have a test case for this somehow because the compilation fails without it. Not sure why constness doesn't matter. Just removed const for now.
Comment 15 Ryosuke Niwa 2021-07-29 15:23:12 PDT
Created attachment 434578 [details]
Patch
Comment 16 Geoffrey Garen 2021-08-02 14:29:30 PDT
Comment on attachment 434578 [details]
Patch

r=me
Comment 17 Ryosuke Niwa 2021-08-02 14:46:33 PDT
Comment on attachment 434578 [details]
Patch

Clearing flags on attachment: 434578

Committed r280559 (240183@main): <https://commits.webkit.org/240183@main>
Comment 18 Ryosuke Niwa 2021-08-02 14:46:36 PDT
All reviewed patches have been landed.  Closing bug.