Summary: | Base GOwnPtr on std::unique_ptr | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zan Dobersek <zan> | ||||||||
Component: | New Bugs | Assignee: | Zan Dobersek <zan> | ||||||||
Status: | RESOLVED DUPLICATE | ||||||||||
Severity: | Normal | CC: | benjamin, cgarcia, cmarcelo, commit-queue, gtk-ews, gyuyoung.kim, mrobinson, rakuco, sam, xan.lopez | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Zan Dobersek
2013-12-26 23:30:23 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.
This works well if std::unique_gptr<T> is a struct that's derived from std::unique_ptr<T, gobject_deleter<T>>. Created attachment 220243 [details]
Patch
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.
It doesn't seem right to put this in the std namespace. Comment on attachment 220243 [details]
Patch
It's not OK to add things to the std namespace. Patch looks good otherwise though.
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. Created attachment 221250 [details]
Patch
Comment on attachment 221250 [details] Patch Attachment 221250 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/5201449134850048 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>>; Oh, I didn't know this bug existed :( 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? 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 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); (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. *** This bug has been marked as a duplicate of bug 127170 *** |