Bug 227164 – Add CheckedRef

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Ryosuke Niwa]

Add CheckedRef, which is a reference version of CheckedPtr.

[Ryosuke Niwa]

Created attachment 431805 [details]
Patch

[Ryosuke Niwa]

Created attachment 431815 [details]
Updated for ToT

[Fujii Hironori]

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))

[Fujii Hironori]

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?

[Fujii Hironori]

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?

[Radar WebKit Bug Importer]

<rdar://problem/79770147>

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 434505 [details]
Patch

[Simon Fraser (smfr)]

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.

[Darin Adler]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

(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.

[Darin Adler]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 434578 [details]
Patch

[Geoffrey Garen]

Comment on attachment 434578 [details]
Patch

r=me

[Ryosuke Niwa]

Comment on attachment 434578 [details]
Patch

Clearing flags on attachment: 434578

Committed r280559 (240183@main): <https://commits.webkit.org/240183@main>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.