Summary: | Add CheckedRef | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||
Component: | Web Template Framework | Assignee: | 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
Ryosuke Niwa
2021-06-18 03:22:05 PDT
Created attachment 431805 [details]
Patch
Created attachment 431815 [details]
Updated for ToT
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 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 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 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. Created attachment 434505 [details]
Patch
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 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. (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. (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. 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. (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. Created attachment 434578 [details]
Patch
Comment on attachment 434578 [details]
Patch
r=me
Comment on attachment 434578 [details] Patch Clearing flags on attachment: 434578 Committed r280559 (240183@main): <https://commits.webkit.org/240183@main> All reviewed patches have been landed. Closing bug. |