Bug 62642

Created attachment 97127 [details] Patch

As discussed in the bugs related to upstreaming the EFL port's DumpRenderTree (such as [1]), it would be very good to have some smart pointer class to manage our char* allocations (as well as some others such as our Evas_Object pointers). Since the memory we use is allocated with malloc(), we cannot rely on OwnPtr, which uses operator delete(); GOwnPtr uses g_free() on its turn. It will be possible to use it in our WebKit port itself too, shortening our error handling and memory management code quite a bit. [1] https://bugs.webkit.org/show_bug.cgi?id=62034#c2

Created attachment 97130 [details] Patch

The style bot will probably complain about EflOwnPtr.h, but we cannot avoid calling Evas_Object Evas_Object :)

Attachment 97130 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/wtf/efl/EflOwnPtr.h:34: Evas_Object is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 97131 [details] Patch

Sorry, this should hopefully be the last version. False positive style alarm still included.

Attachment 97131 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/wtf/efl/EflOwnPtr.h:34: Evas_Object is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 97131 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97131&action=review > Source/JavaScriptCore/ChangeLog:7 > + [Efl] Add EflOwnPtr.h. > + https://bugs.webkit.org/show_bug.cgi?id=62642 > + The name EflOwnPtr looks very weird to me. Also, I'm thinking if we couldn't share this implementation with GTK, with #ifdef on the different parts since they are very similar.

(In reply to comment #9) > The name EflOwnPtr looks very weird to me. EOwnPtr looks worse IMO. > Also, I'm thinking if we couldn't share this implementation with GTK, with #ifdef on the different parts since they are very similar. Couldn't we do that later after discussing this with the GTK guys?

To make this even more explicit, as asked by Lucas: it is not that we do not want to collaborate with the GTK folks, but merging these classes would require either renaming GOwnPtr, moving it out of gobject/ and #ifdef'ing the g*-dependent code or unconditionally including gobject/ in EFL's CMakeLists.txts and then #ifdef'ing the g*-dependent code in GOwnPtr. These changes may look valid, but could certainly be done in a later stage; right now it'd be good to have this in so that the other 5 bug reports related to upstreaming DumpRenderTree and ImageDiff are unblocked.

(In reply to comment #11) > To make this even more explicit, as asked by Lucas: it is not that we do not want to collaborate with the GTK folks, but merging these classes would require either renaming GOwnPtr, moving it out of gobject/ and #ifdef'ing the g*-dependent code or unconditionally including gobject/ in EFL's CMakeLists.txts and then #ifdef'ing the g*-dependent code in GOwnPtr. > > These changes may look valid, but could certainly be done in a later stage; right now it'd be good to have this in so that the other 5 bug reports related to upstreaming DumpRenderTree and ImageDiff are unblocked. It seems reasonable. Informal r+.

Comment on attachment 97131 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97131&action=review > Source/JavaScriptCore/wtf/efl/EflOwnPtr.h:39 > +template <> void freeOwnedEflPtr<Evas_Object>(Evas_Object*); Can't we just add this one function to OwnPtr and then OwnPtr will work correctly for EFL objects? > Source/JavaScriptCore/wtf/efl/EflOwnPtr.h:132 > +template <typename T> inline void freeOwnedEflPtr(T* ptr) > +{ > + std::free(ptr); > +} I think we just need this type specialization for OwnPtr and then OwnPtr will work like a charm for what you want.

(In reply to comment #13) > > Source/JavaScriptCore/wtf/efl/EflOwnPtr.h:132 > > +template <typename T> inline void freeOwnedEflPtr(T* ptr) > > +{ > > + std::free(ptr); > > +} > > I think we just need this type specialization for OwnPtr and then OwnPtr will work like a charm for what you want. Sorry, I don't get this part -- there is no specialization in this section, how would OwnPtr know when to call 'delete ptr' and 'free(ptr)'?

Comment on attachment 97131 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97131&action=review > Source/JavaScriptCore/wtf/efl/EflOwnPtr.cpp:32 > +{ > + evas_object_del(ptr); > +} Are Evas Objects always reference counted [1]? If so it would probably be more correct to specialize RefPtr instead. 1. http://docs.enlightenment.org/auto/evas/group__Evas__Object__Group__Basic.html#gaa22895e88e2cb157b9c38a6d3ce10723 >> Source/JavaScriptCore/wtf/efl/EflOwnPtr.h:132 >> +} > > I think we just need this type specialization for OwnPtr and then OwnPtr will work like a charm for what you want. Totally agree with Eric here. Just be sure to always use new when allocating memory with OwnPtr.

(In reply to comment #15) > >> Source/JavaScriptCore/wtf/efl/EflOwnPtr.h:132 > >> +} > > > > I think we just need this type specialization for OwnPtr and then OwnPtr will work like a charm for what you want. > > Totally agree with Eric here. Just be sure to always use new when allocating memory with OwnPtr. That's the problem -- I'm using *OwnPtr to manage pointers created by EFL itself, so there's no new involved whatsoever.

(In reply to comment #16) > That's the problem -- I'm using *OwnPtr to manage pointers created by EFL itself, so there's no new involved whatsoever. Are these pointers that are not Evas_Object*?

Yes. Most of the time I'm thinking of char*s here.

(In reply to comment #18) > Yes. Most of the time I'm thinking of char*s here. If you must have a specialized OwnPtr, we can probably share most of the code with GOwnPtr.

(In reply to comment #19) > If you must have a specialized OwnPtr, we can probably share most of the code with GOwnPtr. I think so, too. But I still don't know what to do in the general case, such as char*'s: do you suggest making GOwnPtr's default freeOwnedGPtr call g_free() or free() depending on the platform?

(In reply to comment #20) > (In reply to comment #19) > > If you must have a specialized OwnPtr, we can probably share most of the code with GOwnPtr. > > I think so, too. But I still don't know what to do in the general case, such as char*'s: do you suggest making GOwnPtr's default freeOwnedGPtr call g_free() or free() depending on the platform? Yes, that could work.

(In reply to comment #21) > (In reply to comment #20) > > (In reply to comment #19) > > > If you must have a specialized OwnPtr, we can probably share most of the code with GOwnPtr. > > > > I think so, too. But I still don't know what to do in the general case, such as char*'s: do you suggest making GOwnPtr's default freeOwnedGPtr call g_free() or free() depending on the platform? > > Yes, that could work. Good. Going back to comment #11, do you think it makes sense to either move GOwnPtr out of gobject/ to make it more general? How about specializing OwnPtr for GError, GList and the rest and removing the GOwnPtr specializations for them?

(In reply to comment #22) > Good. Going back to comment #11, do you think it makes sense to either move GOwnPtr out of gobject/ to make it more general? How about specializing OwnPtr for GError, GList and the rest and removing the GOwnPtr specializations for them? This could probably be several patches. 1. Convert the GOwnPtr specializations to OwnPtr specializations. 2. Create a smart pointer to be used for types that need to call free/g_free. We could probably start afresh from OwnFastMallocPtr.h, which is very light-weight. Thoughts?

(In reply to comment #23) > (In reply to comment #22) > > > Good. Going back to comment #11, do you think it makes sense to either move GOwnPtr out of gobject/ to make it more general? How about specializing OwnPtr for GError, GList and the rest and removing the GOwnPtr specializations for them? > > This could probably be several patches. > > 1. Convert the GOwnPtr specializations to OwnPtr specializations. Sounds reasonable. I started playing around with this, but I'm getting some errors because GOwnPtr has outPtr(), which OwnPtr does not. I wonder if such addition to OwnPtr would be welcome. > 2. Create a smart pointer to be used for types that need to call free/g_free. We could probably start afresh from OwnFastMallocPtr.h, which is very light-weight. Looking at it, using OwnFastMallocPtr itself would already solve my char* management issues. I'm open to ideas on where to put g_free() in the code flow.

(In reply to comment #24) > > 2. Create a smart pointer to be used for types that need to call free/g_free. We could probably start afresh from OwnFastMallocPtr.h, which is very light-weight. > > Looking at it, using OwnFastMallocPtr itself would already solve my char* management issues. I'm open to ideas on where to put g_free() in the code flow. It seems like this would only be the case if TCMalloc was disabled. fastFree and fastAlloc work via TCMalloc.

(In reply to comment #25) > (In reply to comment #24) > > > > 2. Create a smart pointer to be used for types that need to call free/g_free. We could probably start afresh from OwnFastMallocPtr.h, which is very light-weight. > > > > Looking at it, using OwnFastMallocPtr itself would already solve my char* management issues. I'm open to ideas on where to put g_free() in the code flow. > > It seems like this would only be the case if TCMalloc was disabled. fastFree and fastAlloc work via TCMalloc. Oh well :) Given that OwnFastMallocPtr doesn't seem to be very well-suited for inheritance (no virtual destructor, for example), do you suggest starting a new smart pointer class based on it (but not inheriting from it)?

(In reply to comment #26) > Oh well :) > > Given that OwnFastMallocPtr doesn't seem to be very well-suited for inheritance (no virtual destructor, for example), do you suggest starting a new smart pointer class based on it (but not inheriting from it)? Yes, though it does seem as if GOwnPtr has some nice features as well. It's up to you in the end whether to make the shared class based on GOwnPtr or OwnFastMallocPtr. Whatever seems more reasonable to you.

So it looks like it will be more difficult to merge our smart pointer needs with GOwnPtr than expected. I also can't just add some specializations for Evas_Object and friends to OwnPtr to these object types because the actual definitions of Evas_Object, Ecore_Evas & co. is actually done in a private header which is not installed by the EFL, which breaks operator* and other methods which need the actual structs. So how do you all folks feel about going back to the original EflOwnPtr idea which just calls free() on the common case, has specializations for EFL structs and works without needing the struct definitions (ie. no references to T, but only to T*)?

After working on this a bit more, I actually got some OwnPtr specializations for EFL objects working, and OwnFastMallocPtr looks enough for our char* needs. I'm closing this bug and will open a new one with our OwnPtr specializations shortly.

I appreciate you taking the time to get this all right. :) thanks.