[webkit-reviews] review granted: [Bug 215936] Add CompactUniquePtrTuple : [Attachment 407870] Addresed Darin's review comment
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Sep 2 23:16:17 PDT 2020
Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 215936: Add CompactUniquePtrTuple
https://bugs.webkit.org/show_bug.cgi?id=215936
Attachment 407870: Addresed Darin's review comment
https://bugs.webkit.org/attachment.cgi?id=407870&action=review
--- Comment #13 from Darin Adler <darin at apple.com> ---
Comment on attachment 407870
--> https://bugs.webkit.org/attachment.cgi?id=407870
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...
More information about the webkit-reviews
mailing list