On 01/05/13 15:27, daniel.za...@samsung.com wrote:
> On 05/01/2013 04:59 PM, Tom Hacohen wrote:
>> On 01/05/13 09:13, daniel.za...@samsung.com wrote:
>>> Hi all,
>>>
>>> I pushed today the data referencing feature for Eo objects in efl and
>>> elementary. Check the commit log for explanations on why/how we do that.
>>>
>>> From now, eo_data_get is deprecated, so please use eo_data_scope_get,
>>> eo_data_ref or eo_data_xref instead.
>> Comments:
>> 1. Super spank, you've included unrelated changes!!!
>>
>> @@ -1240,7 +1244,7 @@ eo_xunref(Eo *obj_id, const Eo *ref_obj_id)
>> Eo_Xref_Node *xref = NULL;
>> EINA_INLIST_FOREACH(obj->xrefs, xref)
>> {
>> - if (xref->ref_obj == ref_obj)
>> + if (xref->ref_obj == ref_obj_id)
>>
>> Fixes another issue that you've introduced (or so it seems).
> Super spank on this!!! So yes, it's a fix of the Eo Id commit. And what?
> Spank because it is on the same commit. For that, I call you "enculeur
> de mouches". Learn, it is a new french expression for you.
Dude, seriously that's *SO* not nit-picking. If there's anything
important about committing is this. People may want to cherry-pick
commits that fix stuff, but wouldn't want to introduce new issues.
People (me?) might have wanted to violently revert this commit. Would
have been really annoying had I had to commit the fix manually
afterwards. Really, if there's one important thing, it's this.
>>
>> 2. I don't like the way you handle the data. You keep a refcount for it,
>> but you actually delete the data even if the reference is non-0. You
>> should do it like a true reference count.
>> You should free the data once the reference count hits 0.
>> You should always maintain at least one reference count (the object
>> referencing it's own data) so when the object is deleted that will get
>> decremented and the data will be freed or kept according to the
>> maintained reference count.
> I will check it. For the moment, EO_DEBUG is disabled (otherwise, we
> will have to spank the entire world for the errors that will appear).
> The most important was to push the new APIs for the release. The next
> phase of the data referencing will focus on this part.
Has nothing to do with EO_DEBUG... It's just the count itself which
needs fixing. As you mentioned, the API is more important, nonetheless,
this should be fixed.
>>
>> 3. +_eo_data_xref_internal
>> You return uninitialised data if klass is NULL.
>> I know your code in it's current iteration won't get there, but still,
>> initialise it, or remove the klass!=NULL check if you think it should
>> never happen. At the moment it's unclear.
> Even if we don't use it, I had to initialize it. I will fix it.
Cool.
>> 4. I don't understand how this even get in. You didn't add tests at all.
>> It's Eo, in Eo we actually test shit. I'm really tempted to just revert
>> it. Especially since my e got slow and annoying since your changes
>> (among others, haven't checked which).
> So revert. I will not push it back. And before accusing me of making
> your E slow, just check WHICH COMMITS ARE AROUND MINE!!!!!!!!!!!!!!!!!
> Check, not the name but the contents of each.
I didn't say it was you, and as I told you on IRC (or xmpp, can't
remember), I'm looking into that. It actually doesn't look like you.
That was just an additional comment, the most important part is the
inclusion of testing, which you have SERIOUSLY fucked up. As I've said,
Eo is CORE if Eo breaks, everything breaks. Eo should be tested. Well
everything should be tested, but Eo must be tested. Also, Eo is already
well tested, don't ruin that.
>>
>> Bottom line. Fix it. It needs fixing. Especially test stuff. Though the
>> general patch looks good and clean.
> As usual, the good guy sentence!
I'm a schizophrenic good-cop, bad-cop, you know that.
--
Tom.
------------------------------------------------------------------------------
Introducing AppDynamics Lite, a free troubleshooting tool for Java/.NET
Get 100% visibility into your production application - at no cost.
Code-level diagnostics for performance bottlenecks with <2% overhead
Download for free and get started troubleshooting in minutes.
http://p.sf.net/sfu/appdyn_d2d_ap1
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
