Bug 62642 - [Efl] Add EflOwnPtr to wtf
Summary: [Efl] Add EflOwnPtr to wtf
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 61958
  Show dependency treegraph
 
Reported: 2011-06-14 09:03 PDT by Raphael Kubo da Costa (:rakuco)
Modified: 2011-06-17 10:30 PDT (History)
7 users (show)

See Also:


Attachments
Patch (6.91 KB, patch)
2011-06-14 09:07 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Patch (7.33 KB, patch)
2011-06-14 09:21 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Patch (7.34 KB, patch)
2011-06-14 09:55 PDT, Raphael Kubo da Costa (:rakuco)
mrobinson: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raphael Kubo da Costa (:rakuco) 2011-06-14 09:03:45 PDT
[Efl] Add EflOwnPtr to wtf
Comment 1 Raphael Kubo da Costa (:rakuco) 2011-06-14 09:07:03 PDT
Created attachment 97127 [details]
Patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2011-06-14 09:10:24 PDT
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
Comment 3 Raphael Kubo da Costa (:rakuco) 2011-06-14 09:21:48 PDT
Created attachment 97130 [details]
Patch
Comment 4 Raphael Kubo da Costa (:rakuco) 2011-06-14 09:22:32 PDT
The style bot will probably complain about EflOwnPtr.h, but we cannot avoid calling Evas_Object Evas_Object :)
Comment 5 WebKit Review Bot 2011-06-14 09:24:54 PDT
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.
Comment 6 Raphael Kubo da Costa (:rakuco) 2011-06-14 09:55:47 PDT
Created attachment 97131 [details]
Patch
Comment 7 Raphael Kubo da Costa (:rakuco) 2011-06-14 09:56:23 PDT
Sorry, this should hopefully be the last version. False positive style alarm still included.
Comment 8 WebKit Review Bot 2011-06-14 09:58:11 PDT
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 9 Lucas De Marchi 2011-06-14 10:46:39 PDT
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.
Comment 10 Raphael Kubo da Costa (:rakuco) 2011-06-15 06:21:44 PDT
(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?
Comment 11 Raphael Kubo da Costa (:rakuco) 2011-06-15 06:39:17 PDT
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.
Comment 12 Lucas De Marchi 2011-06-15 07:19:26 PDT
(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 13 Eric Seidel (no email) 2011-06-15 07:29:06 PDT
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.
Comment 14 Raphael Kubo da Costa (:rakuco) 2011-06-15 07:43:36 PDT
(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 15 Martin Robinson 2011-06-15 07:54:02 PDT
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.
Comment 16 Raphael Kubo da Costa (:rakuco) 2011-06-15 07:57:35 PDT
(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.
Comment 17 Martin Robinson 2011-06-15 08:02:15 PDT
(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*?
Comment 18 Raphael Kubo da Costa (:rakuco) 2011-06-15 08:05:43 PDT
Yes. Most of the time I'm thinking of char*s here.
Comment 19 Martin Robinson 2011-06-15 08:38:11 PDT
(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.
Comment 20 Raphael Kubo da Costa (:rakuco) 2011-06-15 09:07:01 PDT
(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?
Comment 21 Martin Robinson 2011-06-15 09:08:54 PDT
(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.
Comment 22 Raphael Kubo da Costa (:rakuco) 2011-06-15 09:14:37 PDT
(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?
Comment 23 Martin Robinson 2011-06-15 09:19:55 PDT
(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?
Comment 24 Raphael Kubo da Costa (:rakuco) 2011-06-15 10:41:57 PDT
(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.
Comment 25 Martin Robinson 2011-06-15 11:00:47 PDT
(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.
Comment 26 Raphael Kubo da Costa (:rakuco) 2011-06-16 07:19:26 PDT
(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)?
Comment 27 Martin Robinson 2011-06-16 07:35:33 PDT
(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.
Comment 28 Raphael Kubo da Costa (:rakuco) 2011-06-16 12:43:13 PDT
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*)?
Comment 29 Raphael Kubo da Costa (:rakuco) 2011-06-17 10:21:57 PDT
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.
Comment 30 Eric Seidel (no email) 2011-06-17 10:30:44 PDT
I appreciate you taking the time to get this all right. :)  thanks.