Bug 181062 – Add WTF::PoisonedUniquePtr to replace std::unique_ptr when poisoning is desired.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Patch coming.

[Radar WebKit Bug Importer]

<rdar://problem/36167040>

[Mark Lam]

Created attachment 330047 [details]
proposed patch.

[EWS Watchlist]

Attachment 330047 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:38:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:116:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:117:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 330047 [details]
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/PoisonedUniquePtrForNonTriviallyDestructibleArrays.cpp:44
> +    ~Logger() { (*destructCount)++; }

pre-increment?

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

I do not believe we need the static here.

[Mark Lam]

Created attachment 330063 [details]
patch for landing.

Thanks for the review.  I've updated the patch with the feedback.  Wil land when the EWS bots are green.

[EWS Watchlist]

Attachment 330063 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/PoisonedUniquePtr.h:116:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Landed in r226247: <http://trac.webkit.org/r226247>.