[webkit-reviews] review granted: [Bug 181062] Add WTF::PoisonedUniquePtr to replace std::unique_ptr when poisoning is desired. : [Attachment 330047] proposed patch.

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 181062: Add WTF::PoisonedUniquePtr to replace std::unique_ptr when
poisoning is desired.
https://bugs.webkit.org/show_bug.cgi?id=181062

Attachment 330047: proposed patch.

https://bugs.webkit.org/attachment.cgi?id=330047&action=review




--- Comment #4 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 330047
  --> https://bugs.webkit.org/attachment.cgi?id=330047
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=330047&action=review

r=me

> Source/WTF/wtf/PoisonedUniquePtr.h:38
> +    PoisonedUniquePtr() : Base() { }

PoisonedUniquePtr() = default; ?

> Source/WTF/wtf/PoisonedUniquePtr.h:57
> +	   return PoisonedUniquePtr(result);

Seems like return result; would just work since this constructor is not marked
as explicit.

> Source/WTF/wtf/PoisonedUniquePtr.h:72
> +	   ASSERT(this == reinterpret_cast<void*>(&ptr) || this->unpoisoned()
!= ptr.unpoisoned());

I believe reinterpret_cast<void*>() can change the address, no? wouldn't
static_cast<>() be more adequate?

> Source/WTF/wtf/PoisonedUniquePtr.h:73
> +	   if (LIKELY(this != reinterpret_cast<void*>(&ptr))) {

ditto.

> Source/WTF/wtf/PoisonedUniquePtr.h:116
> +    PoisonedUniquePtr() : Base() { }

= default; ?

> Source/WTF/wtf/PoisonedUniquePtr.h:152
> +	   ASSERT(this == reinterpret_cast<void*>(&ptr) || this->unpoisoned()
!= ptr.unpoisoned());

same comment about static_cast.

> Source/WTF/wtf/PoisonedUniquePtr.h:153
> +	   if (LIKELY(this != reinterpret_cast<void*>(&ptr))) {

ditto.

> Tools/TestWebKitAPI/Tests/WTF/PoisonedUniquePtr.cpp:43
> +    ~Logger() { destructCount++; }

I believe we usually pre-increment when possible.

>
Tools/TestWebKitAPI/Tests/WTF/PoisonedUniquePtrForNonTriviallyDestructibleArray
s.cpp:44
> +    ~Logger() { (*destructCount)++; }

pre-increment?

>
Tools/TestWebKitAPI/Tests/WTF/PoisonedUniquePtrForNonTriviallyDestructibleArray
s.cpp:59
> +static const int arraySize = 2;

I do not believe we need the static here.