Bug 199579 - [WPE][GTK] GUniqueOutPtr::release should return a raw pointer
Summary: [WPE][GTK] GUniqueOutPtr::release should return a raw pointer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-08 11:06 PDT by Michael Catanzaro
Modified: 2019-07-09 23:02 PDT (History)
12 users (show)

See Also:


Attachments
Patch (7.37 KB, patch)
2019-07-08 11:09 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews215 for win-future (13.66 MB, application/zip)
2019-07-08 12:14 PDT, Build Bot
no flags Details
Patch (7.50 KB, patch)
2019-07-09 14:15 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2019-07-08 11:06:03 PDT
GUniqueOutPtr::release should release to a raw pointer, rather than a GUniquePtr. If a GUniquePtr is desired, it's trivial to construct one from the raw pointer, but all existing callsites under Source/ would rather have a raw pointer. Currently they have to call release().release() to get the raw pointer, which is annoying.
Comment 1 Michael Catanzaro 2019-07-08 11:09:53 PDT
Created attachment 373644 [details]
Patch
Comment 2 Build Bot 2019-07-08 11:15:18 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Build Bot 2019-07-08 12:14:30 PDT Comment hidden (spam)
Comment 4 Build Bot 2019-07-08 12:14:33 PDT Comment hidden (spam)
Comment 5 Adrian Perez 2019-07-08 14:16:25 PDT
Comment on attachment 373644 [details]
Patch

As somebody who once had to use a GUniqueOutPtr with a GError
and call .release().release() on it, I am all in favor of having
this landed :)
Comment 6 Michael Catanzaro 2019-07-08 14:33:49 PDT
Comment on attachment 373644 [details]
Patch

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

> Source/WTF/wtf/glib/GUniquePtr.h:85
>          GUniquePtr<T> ptr(m_ptr);
>          m_ptr = nullptr;
> -        return ptr;
> +        return ptr.release();

I guess it'd be more efficient to do:

T* ptr = m_ptr.release();
m_ptr = nullptr;
return ptr;

to cut out the extra GUniquePtr. I'll update that before landing.
Comment 7 Adrian Perez 2019-07-09 00:48:01 PDT
(In reply to Michael Catanzaro from comment #6)
> Comment on attachment 373644 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373644&action=review
> 
> > Source/WTF/wtf/glib/GUniquePtr.h:85
> >          GUniquePtr<T> ptr(m_ptr);
> >          m_ptr = nullptr;
> > -        return ptr;
> > +        return ptr.release();
> 
> I guess it'd be more efficient to do:
> 
> T* ptr = m_ptr.release();
> m_ptr = nullptr;
> return ptr;
> 
> to cut out the extra GUniquePtr. I'll update that before landing.

Wouldn't that be premature optimization? To me it looks like the
compiler should be smart enough to expand the GUniquePtr template
inline and optimize the code to end up with the same assembler code
for both versions 🤔 (unless -O0 is used, of course).
Comment 8 Carlos Garcia Campos 2019-07-09 00:57:20 PDT
Comment on attachment 373644 [details]
Patch

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

>> Source/WTF/wtf/glib/GUniquePtr.h:85
>> +        return ptr.release();
> 
> I guess it'd be more efficient to do:
> 
> T* ptr = m_ptr.release();
> m_ptr = nullptr;
> return ptr;
> 
> to cut out the extra GUniquePtr. I'll update that before landing.

m_ptr.release() is not valid, since m_ptr is the raw pointer.

I think you could just do:

return std::exchange(m_ptr, nullptr);
Comment 9 Michael Catanzaro 2019-07-09 07:52:36 PDT
(In reply to Carlos Garcia Campos from comment #8)
> m_ptr.release() is not valid, since m_ptr is the raw pointer.
> 
> I think you could just do:
> 
> return std::exchange(m_ptr, nullptr);

Yes!
Comment 10 Michael Catanzaro 2019-07-09 14:15:11 PDT
Created attachment 373764 [details]
Patch
Comment 11 WebKit Commit Bot 2019-07-09 23:02:39 PDT
Comment on attachment 373764 [details]
Patch

Clearing flags on attachment: 373764

Committed r247294: <https://trac.webkit.org/changeset/247294>
Comment 12 WebKit Commit Bot 2019-07-09 23:02:41 PDT
All reviewed patches have been landed.  Closing bug.