Summary: | [WPE][GTK] GUniqueOutPtr::release should return a raw pointer | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||
Component: | WebKitGTK | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aperez, benjamin, berto, bugs-noreply, cdumez, cgarcia, cmarcelo, commit-queue, dbates, ews-watchlist, gustavo, mcatanzaro | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=199572 | ||||||||||
Attachments: |
|
Description
Michael Catanzaro
2019-07-08 11:06:03 PDT
Created attachment 373644 [details]
Patch
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 on attachment 373644 [details] Patch Attachment 373644 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12691399 New failing tests: fast/block/float/float-with-anonymous-previous-sibling.html Created attachment 373654 [details]
Archive of layout-test-results from ews215 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
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 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. (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 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); (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! Created attachment 373764 [details]
Patch
Comment on attachment 373764 [details] Patch Clearing flags on attachment: 373764 Committed r247294: <https://trac.webkit.org/changeset/247294> All reviewed patches have been landed. Closing bug. |