Bug 62642 – [Efl] Add EflOwnPtr to wtf

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Raphael Kubo da Costa (:rakuco)]

[Efl] Add EflOwnPtr to wtf

[Raphael Kubo da Costa (:rakuco)]

Created attachment 97127 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

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

[Raphael Kubo da Costa (:rakuco)]

Created attachment 97130 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

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

[WebKit Review Bot]

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.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 97131 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

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

[WebKit Review Bot]

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.

[Lucas De Marchi]

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.

[Raphael Kubo da Costa (:rakuco)]

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

[Raphael Kubo da Costa (:rakuco)]

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.

[Lucas De Marchi]

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

[Eric Seidel (no email)]

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.

[Raphael Kubo da Costa (:rakuco)]

(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)'?

[Martin Robinson]

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.

[Raphael Kubo da Costa (:rakuco)]

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

[Martin Robinson]

(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*?

[Raphael Kubo da Costa (:rakuco)]

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

[Martin Robinson]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Martin Robinson]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Martin Robinson]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Martin Robinson]

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

[Raphael Kubo da Costa (:rakuco)]

(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)?

[Martin Robinson]

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

[Raphael Kubo da Costa (:rakuco)]

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*)?

[Raphael Kubo da Costa (:rakuco)]

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.

[Eric Seidel (no email)]

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