Bug 181062 - Add WTF::PoisonedUniquePtr to replace std::unique_ptr when poisoning is desired.
Summary: Add WTF::PoisonedUniquePtr to replace std::unique_ptr when poisoning is desired.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-20 16:08 PST by Mark Lam
Modified: 2017-12-21 15:06 PST (History)
11 users (show)

See Also:


Attachments
proposed patch. (71.89 KB, patch)
2017-12-21 12:02 PST, Mark Lam
cdumez: review+
Details | Formatted Diff | Diff
patch for landing. (71.74 KB, patch)
2017-12-21 14:08 PST, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2017-12-20 16:08:19 PST
Patch coming.
Comment 1 Radar WebKit Bug Importer 2017-12-20 16:09:10 PST
<rdar://problem/36167040>
Comment 2 Mark Lam 2017-12-21 12:02:05 PST
Created attachment 330047 [details]
proposed patch.
Comment 3 EWS Watchlist 2017-12-21 12:04:55 PST
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.
Comment 4 Chris Dumez 2017-12-21 13:19:36 PST
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.
Comment 5 Mark Lam 2017-12-21 14:08:09 PST
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.
Comment 6 EWS Watchlist 2017-12-21 14:11:11 PST
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.
Comment 7 Mark Lam 2017-12-21 15:06:05 PST
Landed in r226247: <http://trac.webkit.org/r226247>.