Page MenuHomePhabricator

eo/vtable: refactor vtable handling code for easy maintenance and readability.
Needs ReviewPublic

Authored by smohanty on Aug 29 2019, 7:15 PM.

Details

Summary
  • simplyfied _vtable_chain_merge() function
  • moved function table merge to its own helper function.
Test Plan
efl eo test suite passed , elementary_test tested.

Diff Detail

Repository
rEFL core/efl
Branch
eo_refcator
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13044
Build 9277: arc lint + arc unit
smohanty created this revision.Aug 29 2019, 7:15 PM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

smohanty requested review of this revision.Aug 29 2019, 7:15 PM

Overall looks fine to me but leave one question just in case.

src/lib/eo/eo.c
156

Question)
func.src is unused? No need to compare?

tasn added a comment.Aug 30 2019, 1:55 AM

I don't get this change. Readability is subjective and this doesn't read any more readable to me, and it's 20 lines longer (not that it's the only parameter, but still).
It's also hard to follow when you changed a few unrelated things in one commit (or is it just phab being bad?).

smohanty added inline comments.Aug 30 2019, 1:57 AM
src/lib/eo/eo.c
156

As this merge is just a copy from src to destination , and if the src function exists we are sure that there is a valid Klass/src exists (as they come as a combo) we can safely copy from src to dest.
Note: if src and dest func are same but klass/src are different then already data is corrupted because we can't have different Klass for same function pointer.

smohanty added inline comments.Aug 30 2019, 2:14 AM
src/lib/eo/eo.c
158

used to call _vtable_chain_write_prepare(dst) in a for loop . which internally calls _vtable_chain_merge(dst, &old);

In D9794#182033, @tasn wrote:

I don't get this change. Readability is subjective and this doesn't read any more readable to me, and it's 20 lines longer (not that it's the only parameter, but still).
It's also hard to follow when you changed a few unrelated things in one commit (or is it just phab being bad?).

Just refactored _vtable_chain_merge() which was calling chain_write_prepare() inside a for loop , which internally has a call to _vtable_chain_merge(). so refactored it out so that the control flow is linear and easy to understand.

smohanty marked an inline comment as done.Aug 30 2019, 2:16 AM
smohanty added inline comments.Aug 30 2019, 2:24 AM
src/lib/eo/eo.c
183

Used to call merge with a newly allocated table (could have done by direct memcopy ).

Hermet accepted this revision.Aug 30 2019, 2:56 AM
This revision is now accepted and ready to land.Aug 30 2019, 2:56 AM
In D9794#182033, @tasn wrote:

I don't get this change. Readability is subjective and this doesn't read any more readable to me, and it's 20 lines longer (not that it's the only parameter, but still).
It's also hard to follow when you changed a few unrelated things in one commit (or is it just phab being bad?).

It could be subtle, but it clearly brings clean and neat code by split functions with better clear descriptions.
Unless you clearly reject, I'd like to accept this.

tasn requested changes to this revision.Aug 30 2019, 3:19 AM
In D9794#182033, @tasn wrote:

I don't get this change. Readability is subjective and this doesn't read any more readable to me, and it's 20 lines longer (not that it's the only parameter, but still).
It's also hard to follow when you changed a few unrelated things in one commit (or is it just phab being bad?).

It could be subtle, but it clearly brings clean and neat code by split functions with better clear descriptions.
Unless you clearly reject, I'd like to accept this.

I don't think it clearly makes anything clean or neat and I don't see the better clear descriptions you're talking about. I'm not trying to be difficult, I really just don't. I think changing code just for the sake of changing is a bad idea, and i think that if everyone start changing code just to better fit their style we would have a mess.

This revision now requires changes to proceed.Aug 30 2019, 3:19 AM
smohanty requested review of this revision.Aug 30 2019, 5:29 AM

Note for future review comment.

Please give clear description what exact change you need for this patch . could be coding style or test fail or whatever be precise .
I don't want to waste anyones time with needless argument nor mine.

Give a review comment on the code inline . then I will give my reply inline.

Please refrain of giving blanket statement.

Its an open source project I can just change a spelling mistake or variable name and raise a patch thats my prerogative . You have to give review comments on the merit of the patch not whether i can change this part of
the code or not.

tasn added a comment.Aug 30 2019, 5:42 AM

Note for future review comment.

Please give clear description what exact change you need for this patch . could be coding style or test fail or whatever be precise .
I don't want to waste anyones time with needless argument nor mine.

Give a review comment on the code inline . then I will give my reply inline.

Please refrain of giving blanket statement.

Its an open source project I can just change a spelling mistake or variable name and raise a patch thats my prerogative . You have to give review comments on the merit of the patch not whether i can change this part of
the code or not.

Thank you for your useful advice.

You can definitely make patches that do whatever. However, that doesn't mean all contributions are accepted.

I don't think the change itself has any merit, so yes it's a blanket comment, and yet it's very relevant. I can make inline comments on every line and explain why it's worse not better. Some of the changes make it much worse, so I can comment on them directly, but there's no point because again, the whole point of the patch is wrong.
This patch essentially changes the style in the source from whatever is there to the style you prefer. It's a more intrusive equivalent of making a patch that changes for ( i = 0 ; i < 5 ; i++) to for ( itr = 0 ; itr < 5 ; itr++)` because "itr is a much better name, i is confusing". It's just a style choice, and we don't accept patches for style preferences.

As I said in another patch of yours, if you want to change things, the onus is on you to prove that your change is useful. You can make statements like "this is better" which you have, I can disagree with them.

static inline void
_vtable_chain_merge(Dich_Chain1 *dst, const Dich_Chain1 *src)
{
   size_t j;
   const op_type_funcs *sf = src->chain2->funcs;
   op_type_funcs *df = dst->chain2->funcs;

   if (df == sf)
     {
        /* Skip if the chain is the same. */
        return;
     }

   for (j = 0 ; j < DICH_CHAIN_LAST_SIZE ; j++, df++, sf++)
     {
        if (sf->func && memcmp(df, sf, sizeof(*df)))
          {
             _vtable_chain_write_prepare(dst);
             df = dst->chain2->funcs + j;
             memcpy(df, sf, sizeof(*df));
          }
     }
}

can you understand what this function does in isolation ?

its calling _vtable_chain_write_prepare(dst); in a for loop and as the name suggests its preparing for write .. but why its doing inside a loop .. can you understand by not understanding that it also handles ref count and stuff
and inside the _vtable_chain_write_prepare() its calling the caller function (recursive). By changing it to linear flow is a big crime ?

if you compare this refactoring with changing mere for loop .. I don't know what else to say to you.

tasn added a comment.Aug 30 2019, 6:49 AM
static inline void
_vtable_chain_merge(Dich_Chain1 *dst, const Dich_Chain1 *src)
{
   size_t j;
   const op_type_funcs *sf = src->chain2->funcs;
   op_type_funcs *df = dst->chain2->funcs;

   if (df == sf)
     {
        /* Skip if the chain is the same. */
        return;
     }

   for (j = 0 ; j < DICH_CHAIN_LAST_SIZE ; j++, df++, sf++)
     {
        if (sf->func && memcmp(df, sf, sizeof(*df)))
          {
             _vtable_chain_write_prepare(dst);
             df = dst->chain2->funcs + j;
             memcpy(df, sf, sizeof(*df));
          }
     }
}

can you understand what this function does in isolation ?

its calling _vtable_chain_write_prepare(dst); in a for loop and as the name suggests its preparing for write .. but why its doing inside a loop .. can you understand by not understanding that it also handles ref count and stuff
and inside the _vtable_chain_write_prepare() its calling the caller function (recursive). By changing it to linear flow is a big crime ?

if you compare this refactoring with changing mere for loop .. I don't know what else to say to you.

Yes I understand. It's inside the loop because you only want to do it if you have a reason to duplicate, so any of these if (sf->func && memcmp(df, sf, sizeof(*df))) is true. You don't want to do it if it's not the case. The "prepare" function therefore aborts if we don't need to do anything.
Why is it calling the parent? I have no idea, maybe it's just so this function is reusable, because in the case you're describing, it won't do anything anyway. Again, short circuit, though I agree this could be more explicit.

So as suspected, you actually changed behaviour, not just moved code around (though maybe it doesn't matter, I don't know).

Either way, even if this specific change you mentioned is a good idea (not saying that it is) and doesn't break anything (not sure about that), it's hard to spot it or review it because you mixed it with a few other changes, for example renaming "dst" to "obj" (why?! it's not an obj!) and changing how alloc behaves (I'm actually not necessarily against this one, just hard to review with so many changes in one commit).

Another point: the message you just wrote about explaining why you made this change would have been very useful to have at the start and should definitely be in the commit message and in every patch you send from now on. It's very hard to review patches and commits without seeing any explanations as to why things are done.