[webkit-reviews] review granted: [Bug 231819] Add tests for WTF::OSObjectPtr/adoptOSObject in Cocoa with and without ARC : [Attachment 441418] Patch v1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 231819: Add tests for WTF::OSObjectPtr/adoptOSObject in Cocoa with and
without ARC
https://bugs.webkit.org/show_bug.cgi?id=231819

Attachment 441418: Patch v1

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




--- Comment #10 from Darin Adler <darin at apple.com> ---
Comment on attachment 441418
  --> https://bugs.webkit.org/attachment.cgi?id=441418
Patch v1

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

> Source/WTF/wtf/OSObjectPtr.h:42
> +template<typename T> OSObjectPtr<T> adoptOSObject(T) WARN_UNUSED_RETURN;

Not sure how I feel about adding this.

Typically we use WARN_UNUSED_RETURN when it’s a programming error to not use
the return value. Especially when it’s a particularly dangerous one. We haven’t
done this for adoptRef, although we have done for adoptCF and adoptNS. A little
funny to use it on the much less used functions and not on the
nearly-ubiquitous adoptRef.

And to be super-pedantic, I don’t think it is guaranteed to be an error. For
example, if a function has a side effect and also returns an allocated object,
you might need to call adoptOSObject on it and it would be OK to not use the
result, and let it get destroyed. Another way to write the release.

Anyway, I suppose I am OK with it for now.

> Tools/TestWebKitAPI/Tests/WTF/darwin/OSObjectPtr.cpp:66
> +    EXPECT_EQ(1, CFGetRetainCount((CFTypeRef)fooPtr));

In non-ARC cases, doesn’t this test leak a dispatch queue? Doesn’t seem like a
new issue, but also doesn't seem great.

Seems like we’d fix it by adding another WTF::releaseOSObject(foo).