Bug 126255 – Base GOwnPtr on std::unique_ptr

• Component: New Bugs
• Status: RESOLVED DUPLICATE

[Zan Dobersek]

Introduce std::unique_gptr, a GOwnPtr replacement

[Zan Dobersek]

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.

[Zan Dobersek]

This works well if std::unique_gptr<T> is a struct that's derived from std::unique_ptr<T, gobject_deleter<T>>.

[Zan Dobersek]

Created attachment 220243 [details]
Patch

[WebKit Commit Bot]

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.

[Sam Weinig]

It doesn't seem right to put this in the std namespace.

[Anders Carlsson]

Comment on attachment 220243 [details]
Patch

It's not OK to add things to the std namespace. Patch looks good otherwise though.

[Zan Dobersek]

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.

[Zan Dobersek]

Created attachment 221250 [details]
Patch

[kov's GTK+ EWS bot]

Comment on attachment 221250 [details]
Patch

Attachment 221250 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/5201449134850048

[Anders Carlsson]

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>>;

[Carlos Garcia Campos]

Oh, I didn't know this bug existed :(

[Carlos Garcia Campos]

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?

[Carlos Garcia Campos]

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.

[Zan Dobersek]

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);

[Carlos Garcia Campos]

(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.

[Zan Dobersek]

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