I want to have the ability to use free 8-bits in pointer for std::unique_ptr.
Should I wrap unique_ptr around or just write a new template class that behaves like std::unique_ptr?
Not sure how to make something a tuple if you can’t get a reference to one of the things inside.
If it was a moved-out copy of the pointer then clearly unique_ptr is what we’d want. But for a reference to the thing inside the tuple, I don’t see how it can be unique_ptr.
Created attachment 407752 [details] Patch
Comment on attachment 407752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407752&action=review > Source/WTF/ChangeLog:8 > + Added a new template class, CompactUniquePtrTuple, which stores a pointer and up to 16-bits of why just 16? On iOS, we have more free space than that. > Source/WTF/wtf/CompactUniquePtrTuple.h:75 > + template <typename U> > + CompactUniquePtrTuple<T, Type>& operator=(CompactUniquePtrTuple<U, Type>&& other) > + { > + deletePointerIfNotNull(); > + m_data = other.m_data; > + other.m_data = CompactPointerTuple<T*, Type>(); > + return *this; > + } Never sure how to proceed with this, but this doesn't seem right if we end up moving into ourselves. > Source/WTF/wtf/CompactUniquePtrTuple.h:77 > + T* pointer() const { return m_data.pointer(); } call this get() to match unique_ptr API? > Source/WTF/wtf/CompactUniquePtrTuple.h:84 > + std::unique_ptr<T> moveToUniquePtr() > + { > + T* pointer = m_data.pointer(); > + m_data.setPointer(nullptr); > + return std::unique_ptr<T, Deleter>(pointer); > + } maybe call this release? > Source/WTF/wtf/CompactUniquePtrTuple.h:93 > + void setPointer(std::unique_ptr<U>&& pointer) not sure you need && here since this function is taking ownership of this unique_ptr. If you just have it as "std::unique_ptr<U> pointer", the caller will be forced to relinquish ownership
Comment on attachment 407752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407752&action=review > Source/WTF/wtf/CompactUniquePtrTuple.h:37 > +ALWAYS_INLINE CompactUniquePtrTuple<T, Type> makeCompactUniquePtr(Args&&... args) do we want versions of these functions that allow us to pass in Type to the "make" functions?
Comment on attachment 407752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407752&action=review >> Source/WTF/wtf/CompactUniquePtrTuple.h:77 >> + T* pointer() const { return m_data.pointer(); } > > call this get() to match unique_ptr API? maybe this is confusing given it's a tuple. I think pointer() is better
(In reply to Saam Barati from comment #5) > Comment on attachment 407752 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=407752&action=review > > > Source/WTF/ChangeLog:8 > > + Added a new template class, CompactUniquePtrTuple, which stores a pointer and up to 16-bits of > > why just 16? On iOS, we have more free space than that. That's the restriction CompactPointerTuple puts. In general, I don't think it's a good idea to make these classes accept different number of bits in different platforms as we want to share the same code across platforms as much as possible. > > Source/WTF/wtf/CompactUniquePtrTuple.h:75 > > + template <typename U> > > + CompactUniquePtrTuple<T, Type>& operator=(CompactUniquePtrTuple<U, Type>&& other) > > + { > > + deletePointerIfNotNull(); > > + m_data = other.m_data; > > + other.m_data = CompactPointerTuple<T*, Type>(); > > + return *this; > > + } > > Never sure how to proceed with this, but this doesn't seem right if we end > up moving into ourselves. Oh right, I should have moved to a temporary variable first. Will fix. > > Source/WTF/wtf/CompactUniquePtrTuple.h:77 > > + T* pointer() const { return m_data.pointer(); } > > call this get() to match unique_ptr API? What you said. Other Compact*Tuple class use pointer(). > > Source/WTF/wtf/CompactUniquePtrTuple.h:84 > > + std::unique_ptr<T> moveToUniquePtr() > > + { > > + T* pointer = m_data.pointer(); > > + m_data.setPointer(nullptr); > > + return std::unique_ptr<T, Deleter>(pointer); > > + } > > maybe call this release? I think it's in line with pointer/type naming to name this differently from release. I initially called this release as well but saw UniqueRef use moveToUniquePtr. > > Source/WTF/wtf/CompactUniquePtrTuple.h:93 > > + void setPointer(std::unique_ptr<U>&& pointer) > > not sure you need && here since this function is taking ownership of this > unique_ptr. If you just have it as "std::unique_ptr<U> pointer", the caller > will be forced to relinquish ownership That's true but I think it's communicative / self explanatory to use rvalue reference here than to force the creation of a phantom temporary and force the caller to use WTFMove anyway.
Created attachment 407847 [details] Fixed operator=
Comment on attachment 407847 [details] Fixed operator= View in context: https://bugs.webkit.org/attachment.cgi?id=407847&action=review > Source/WTF/wtf/CompactUniquePtrTuple.h:40 > + static_assert(std::is_same<typename T::webkitFastMalloced, int>::value, "T is FastMalloced"); > + return CompactUniquePtrTuple<T, Type>(std::make_unique<T>(*new T(std::forward<Args>(args)...))); Can’t this use WTF::makeUnique instead of repeating its assertion? > Source/WTF/wtf/CompactUniquePtrTuple.h:47 > + static_assert(std::is_same<typename T::webkitFastMalloced, int>::value, "T is FastMalloced"); > + return CompactUniquePtrTuple<T, Type, Deleter>(std::make_unique<T>(*new T(std::forward<Args>(args)...))); Ditto.
(In reply to Darin Adler from comment #10) > Comment on attachment 407847 [details] > Fixed operator= > > View in context: > https://bugs.webkit.org/attachment.cgi?id=407847&action=review > > > Source/WTF/wtf/CompactUniquePtrTuple.h:40 > > + static_assert(std::is_same<typename T::webkitFastMalloced, int>::value, "T is FastMalloced"); > > + return CompactUniquePtrTuple<T, Type>(std::make_unique<T>(*new T(std::forward<Args>(args)...))); > > Can’t this use WTF::makeUnique instead of repeating its assertion? Oh oops, I forgot fo fix that. Doing that now.
Created attachment 407870 [details] Addresed Darin's review comment
Comment on attachment 407870 [details] Addresed Darin's review comment View in context: https://bugs.webkit.org/attachment.cgi?id=407870&action=review > Source/WTF/wtf/CompactUniquePtrTuple.h:59 > + other.m_data = CompactPointerTuple<T*, Type>(); Could we use std::exchange here? It would make this a one-liner and we could use construction syntax rather than assignment syntax. > Source/WTF/wtf/CompactUniquePtrTuple.h:74 > + return *this; Seems like this should be written more simply: auto moved = WTFMove(other); std:swap(m_data, moved.m_data); return *this; Or this more efficient version that does not null out the other but probably you like nulling out the other: std::swap(*this.m_data, other.m_data); return *this; > Source/WTF/wtf/CompactUniquePtrTuple.h:79 > + std::unique_ptr<T> moveToUniquePtr() Seems wrong that this does not use the proper Deleter. > Source/WTF/wtf/CompactUniquePtrTuple.h:86 > + void setPointer(nullptr_t) Is this really needed? Won’t the other setPointer do this just as efficiently? > Source/WTF/wtf/CompactUniquePtrTuple.h:112 > + void deletePointerIfNotNull() Since the built in delete operator does nothing when passed null it seems this could just be named deletePointer > Source/WTF/wtf/CompactUniquePtrTuple.h:119 > + template<typename U, typename E, typename D, class... Args> friend CompactUniquePtrTuple<U, E, D> makeCompactUniquePtr(Args&&... args); Not sure why these use class... rather than typename...
Comment on attachment 407870 [details] Addresed Darin's review comment View in context: https://bugs.webkit.org/attachment.cgi?id=407870&action=review >> Source/WTF/wtf/CompactUniquePtrTuple.h:59 >> + other.m_data = CompactPointerTuple<T*, Type>(); > > Could we use std::exchange here? It would make this a one-liner and we could use construction syntax rather than assignment syntax. I'm not sure what you mean. If you mean that: m_data = WTFMove(other.m_data) then it won't work because WTFMove(other.m_data) is basically a no-op. >> Source/WTF/wtf/CompactUniquePtrTuple.h:74 >> + return *this; > > Seems like this should be written more simply: > > auto moved = WTFMove(other); > std:swap(m_data, moved.m_data); > return *this; > > Or this more efficient version that does not null out the other but probably you like nulling out the other: > > std::swap(*this.m_data, other.m_data); > return *this; That's a good point. Done the variant which clears out "other". >> Source/WTF/wtf/CompactUniquePtrTuple.h:79 >> + std::unique_ptr<T> moveToUniquePtr() > > Seems wrong that this does not use the proper Deleter. Oops, fixed. I had specified this in the return statement but forgot it here. >> Source/WTF/wtf/CompactUniquePtrTuple.h:86 >> + void setPointer(nullptr_t) > > Is this really needed? Won’t the other setPointer do this just as efficiently? Yes, otherwise, we'd get a build error like: CompactUniquePtrTuple.cpp:67:7: error: no matching member function for call to 'setPointer' a.setPointer(nullptr); ~~^~~~~~~~~~ In file included from Tools/TestWebKitAPI/Tests/WTF/CompactUniquePtrTuple.cpp:30: CompactUniquePtrTuple.h:91:10: note: candidate template ignored: could not match 'unique_ptr<type-parameter-0-0, default_delete<type-parameter-0-0> >' against 'nullptr_t' void setPointer(std::unique_ptr<U>&& pointer) ^ The issue is that the compiler can't convert nullptr_t to std::unique_ptr<U>. >> Source/WTF/wtf/CompactUniquePtrTuple.h:112 >> + void deletePointerIfNotNull() > > Since the built in delete operator does nothing when passed null it seems this could just be named deletePointer Sure, done. >> Source/WTF/wtf/CompactUniquePtrTuple.h:119 >> + template<typename U, typename E, typename D, class... Args> friend CompactUniquePtrTuple<U, E, D> makeCompactUniquePtr(Args&&... args); > > Not sure why these use class... rather than typename... Fixed. Not sure what happened there.
Comment on attachment 407870 [details] Addresed Darin's review comment View in context: https://bugs.webkit.org/attachment.cgi?id=407870&action=review >>> Source/WTF/wtf/CompactUniquePtrTuple.h:59 >>> + other.m_data = CompactPointerTuple<T*, Type>(); >> >> Could we use std::exchange here? It would make this a one-liner and we could use construction syntax rather than assignment syntax. > > I'm not sure what you mean. If you mean that: > m_data = WTFMove(other.m_data) > then it won't work because WTFMove(other.m_data) is basically a no-op. I mean: m_data = std::exchange(other.m_data, { });
Comment on attachment 407870 [details] Addresed Darin's review comment View in context: https://bugs.webkit.org/attachment.cgi?id=407870&action=review >>>> Source/WTF/wtf/CompactUniquePtrTuple.h:59 >>>> + other.m_data = CompactPointerTuple<T*, Type>(); >>> >>> Could we use std::exchange here? It would make this a one-liner and we could use construction syntax rather than assignment syntax. >> >> I'm not sure what you mean. If you mean that: >> m_data = WTFMove(other.m_data) >> then it won't work because WTFMove(other.m_data) is basically a no-op. > > I mean: > > m_data = std::exchange(other.m_data, { }); Or actually, more like this: : m_data { std::exchange(other.m_data, { }) }
(In reply to Darin Adler from comment #16) > Comment on attachment 407870 [details] > Addresed Darin's review comment > > View in context: > https://bugs.webkit.org/attachment.cgi?id=407870&action=review > > >>>> Source/WTF/wtf/CompactUniquePtrTuple.h:59 > >>>> + other.m_data = CompactPointerTuple<T*, Type>(); > >>> > >>> Could we use std::exchange here? It would make this a one-liner and we could use construction syntax rather than assignment syntax. > >> > >> I'm not sure what you mean. If you mean that: > >> m_data = WTFMove(other.m_data) > >> then it won't work because WTFMove(other.m_data) is basically a no-op. > > > > I mean: > > > > m_data = std::exchange(other.m_data, { }); > > Or actually, more like this: > > : m_data { std::exchange(other.m_data, { }) } Oh I see what you're saying. Fixed.
Committed r266573: <https://trac.webkit.org/changeset/266573>
<rdar://problem/68318595>