[webkit-reviews] review granted: [Bug 215936] Add CompactUniquePtrTuple : [Attachment 407870] Addresed Darin's review comment

[bugzilla-daemon at webkit.org]

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