Bug 126255 - Base GOwnPtr on std::unique_ptr
Summary: Base GOwnPtr on std::unique_ptr
Status: RESOLVED DUPLICATE of bug 127170
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-26 23:30 PST by Zan Dobersek
Modified: 2014-01-24 02:33 PST (History)
10 users (show)

See Also:


Attachments
Patch (18.30 KB, patch)
2013-12-26 23:38 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (12.23 KB, patch)
2014-01-02 12:35 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (129.08 KB, patch)
2014-01-15 02:36 PST, Zan Dobersek
gtk-ews: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2013-12-26 23:30:23 PST
Introduce std::unique_gptr, a GOwnPtr replacement
Comment 1 Zan Dobersek 2013-12-26 23:38:40 PST
Created attachment 220050 [details]
Patch

Work in progress, relies on C++11 template aliases so this wouldn't work yet for ports still building with GCC 4.6.
Comment 2 Zan Dobersek 2014-01-02 11:33:03 PST
This works well if std::unique_gptr<T> is a struct that's derived from std::unique_ptr<T, gobject_deleter<T>>.
Comment 3 Zan Dobersek 2014-01-02 12:35:30 PST
Created attachment 220243 [details]
Patch
Comment 4 WebKit Commit Bot 2014-01-02 12:37:17 PST
Attachment 220243 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmakefile.list.am', u'Source/WTF/WTF.vcxproj/WTF.vcxproj', u'Source/WTF/WTF.vcxproj/WTF.vcxproj.filters', u'Source/WTF/wtf/PlatformEfl.cmake', u'Source/WTF/wtf/PlatformGTK.cmake', u'Source/WTF/wtf/PlatformNix.cmake', u'Source/WTF/wtf/gobject/UniqueGPtr.cpp', u'Source/WTF/wtf/gobject/UniqueGPtr.h', '--commit-queue']" exit_code: 1
ERROR: Source/WTF/wtf/gobject/UniqueGPtr.cpp:27:  gobject_deleter::operator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/gobject/UniqueGPtr.cpp:32:  gobject_deleter::operator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/gobject/UniqueGPtr.cpp:37:  gobject_deleter::operator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/gobject/UniqueGPtr.cpp:42:  gobject_deleter::operator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/gobject/UniqueGPtr.cpp:47:  gobject_deleter::operator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/gobject/UniqueGPtr.cpp:52:  gobject_deleter::operator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/gobject/UniqueGPtr.cpp:57:  gobject_deleter::operator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/gobject/UniqueGPtr.h:34:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/gobject/UniqueGPtr.h:39:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/gobject/UniqueGPtr.h:44:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/gobject/UniqueGPtr.h:49:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/gobject/UniqueGPtr.h:54:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/gobject/UniqueGPtr.h:59:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/gobject/UniqueGPtr.h:64:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/gobject/UniqueGPtr.h:69:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/gobject/UniqueGPtr.h:84:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/gobject/UniqueGPtr.h:102:  _gptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/gobject/UniqueGPtr.h:103:  _out_gptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 18 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Sam Weinig 2014-01-04 08:48:47 PST
It doesn't seem right to put this in the std namespace.
Comment 6 Anders Carlsson 2014-01-06 12:45:34 PST
Comment on attachment 220243 [details]
Patch

It's not OK to add things to the std namespace. Patch looks good otherwise though.
Comment 7 Zan Dobersek 2014-01-06 13:23:07 PST
The idea was to stay closely tied to std::unique_ptr, but avoiding the std namespace makes sense.

Considering that, the name might as well stay GOwnPtr for now, unless anyone else can propose something better.
Comment 8 Zan Dobersek 2014-01-15 02:36:58 PST
Created attachment 221250 [details]
Patch
Comment 9 kov's GTK+ EWS bot 2014-01-15 05:47:25 PST
Comment on attachment 221250 [details]
Patch

Attachment 221250 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/5201449134850048
Comment 10 Anders Carlsson 2014-01-15 07:26:22 PST
Comment on attachment 221250 [details]
Patch

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

> Source/WTF/wtf/gobject/GOwnPtr.h:83
> +template <typename T>
> +struct GOwnPtr : public std::unique_ptr<T, GObjectDeleter<T>> {
> +    WTF_MAKE_NONCOPYABLE(GOwnPtr);
> +public:
> +    explicit GOwnPtr(T* ptr = nullptr)
> +        : std::unique_ptr<T, GObjectDeleter<T>>(ptr)
>      {
> -        ASSERT(!ptr || m_ptr != ptr);
> -        freeOwnedGPtr(m_ptr);
> -        m_ptr = ptr;
>      }
> +};

I think this can even be something like

template<typename T>
using GOwnPtr<T> = std::unique_ptr<T, GObjectDeleter<T>>;
Comment 11 Carlos Garcia Campos 2014-01-17 05:03:51 PST
Oh, I didn't know this bug existed :(
Comment 12 Carlos Garcia Campos 2014-01-17 05:52:03 PST
Comment on attachment 221250 [details]
Patch

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

I wrote a similar patch in bug #127170, but adding GUniquePtr first, instead of replacing GOwnPtr. I planned to replace it in follow up patches. I have some comments/questions about this approach.

> Source/WTF/wtf/gobject/GOwnPtr.h:36
> +struct GObjectDeleter {

GObjectDeleter sounds confusing to me, because it seems like something to delete GObjects, which are refcounted. I prefer something like GLibPtrDeleter or simply GPtrDeleter.

> Source/WTF/wtf/gobject/GOwnPtr.h:77
> +    WTF_MAKE_NONCOPYABLE(GOwnPtr);

Why making it non copyable? I guess this doesn't allow to use GOwnPtr as return value of a method, no?. My motivation to write my patch was this andersca's comment:

https://bugs.webkit.org/show_bug.cgi?id=127104#c4

Being a unique_ptr we can make it "copyable", and use std::move() to transfer the ownership.

> Source/WTF/wtf/gobject/GOwnPtr.h:102
> +        return m_outPtr;

Where is m_outPtr set?
Comment 13 Carlos Garcia Campos 2014-01-17 06:01:27 PST
Regarding GOutPtr, I think it could be simpler to use a new name for the smart pointer and rename GOwnPtr as GOutPtr and keep using it only for GError.
Comment 14 Zan Dobersek 2014-01-17 07:29:24 PST
Comment on attachment 221250 [details]
Patch

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

>> Source/WTF/wtf/gobject/GOwnPtr.h:36
>> +struct GObjectDeleter {
> 
> GObjectDeleter sounds confusing to me, because it seems like something to delete GObjects, which are refcounted. I prefer something like GLibPtrDeleter or simply GPtrDeleter.

Indeed, such names would fit better.

>> Source/WTF/wtf/gobject/GOwnPtr.h:77
>> +    WTF_MAKE_NONCOPYABLE(GOwnPtr);
> 
> Why making it non copyable? I guess this doesn't allow to use GOwnPtr as return value of a method, no?. My motivation to write my patch was this andersca's comment:
> 
> https://bugs.webkit.org/show_bug.cgi?id=127104#c4
> 
> Being a unique_ptr we can make it "copyable", and use std::move() to transfer the ownership.

GOwnPtr shouldn't be copyable, just like std::unique_ptr is not copyable, and for good reasons -- having two GOwnPtrs/std::unique_ptrs managing the same pointer would lead to a double free when the second smart pointer is deallocated.

With C++11 the return values are moved, so there's no need for copying in that case either.

>> Source/WTF/wtf/gobject/GOwnPtr.h:83
>> +};
> 
> I think this can even be something like
> 
> template<typename T>
> using GOwnPtr<T> = std::unique_ptr<T, GObjectDeleter<T>>;

Yes, template alias can be used now that GCC 4.6 is not supported anymore.

>> Source/WTF/wtf/gobject/GOwnPtr.h:102
>> +        return m_outPtr;
> 
> Where is m_outPtr set?

This operator returns the reference to m_outPtr. The user should then pass the address of the returned reference to the API call, which then dereferences that address and assigns memory to it, so basically this is executed:

m_outPtr = g_strdup(some_string);
Comment 15 Carlos Garcia Campos 2014-01-17 08:28:18 PST
(In reply to comment #14)
> (From update of attachment 221250 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221250&action=review
> 
> >> Source/WTF/wtf/gobject/GOwnPtr.h:36
> >> +struct GObjectDeleter {
> > 
> > GObjectDeleter sounds confusing to me, because it seems like something to delete GObjects, which are refcounted. I prefer something like GLibPtrDeleter or simply GPtrDeleter.
> 
> Indeed, such names would fit better.
> 
> >> Source/WTF/wtf/gobject/GOwnPtr.h:77
> >> +    WTF_MAKE_NONCOPYABLE(GOwnPtr);
> > 
> > Why making it non copyable? I guess this doesn't allow to use GOwnPtr as return value of a method, no?. My motivation to write my patch was this andersca's comment:
> > 
> > https://bugs.webkit.org/show_bug.cgi?id=127104#c4
> > 
> > Being a unique_ptr we can make it "copyable", and use std::move() to transfer the ownership.
> 
> GOwnPtr shouldn't be copyable, just like std::unique_ptr is not copyable, and for good reasons -- having two GOwnPtrs/std::unique_ptrs managing the same pointer would lead to a double free when the second smart pointer is deallocated.
> 
> With C++11 the return values are moved, so there's no need for copying in that case either.

Yes, but with this patch using WTF_MAKE_NONCOPYABLE the compiler fails when using a GOwnPtr as return value of a function even using std::move(). Or it done differently and I don't know how? C++ is hard.

> >> Source/WTF/wtf/gobject/GOwnPtr.h:83
> >> +};
> > 
> > I think this can even be something like
> > 
> > template<typename T>
> > using GOwnPtr<T> = std::unique_ptr<T, GObjectDeleter<T>>;
> 
> Yes, template alias can be used now that GCC 4.6 is not supported anymore.

Didn't know what template alias was, I'll try that way.

> >> Source/WTF/wtf/gobject/GOwnPtr.h:102
> >> +        return m_outPtr;
> > 
> > Where is m_outPtr set?
> 
> This operator returns the reference to m_outPtr. The user should then pass the address of the returned reference to the API call, which then dereferences that address and assigns memory to it, so basically this is executed:
> 
> m_outPtr = g_strdup(some_string);

The current GOwnPtr is a lot easier to understand, I think.
Comment 16 Zan Dobersek 2014-01-24 02:33:17 PST

*** This bug has been marked as a duplicate of bug 127170 ***