[prev in list] [next in list] [prev in thread] [next in thread] 

List:       enlightenment-devel
Subject:    Re: [E-devel] EET minor details
From:       "Cedric BAIL" <cedric.bail () free ! fr>
Date:       2008-04-28 16:05:13
Message-ID: 7625e9d90804280905h1c3f7b1cqd15c618924ad5185 () mail ! gmail ! com
[Download RAW message or body]

On Mon, Apr 28, 2008 at 5:04 PM, Gustavo Sverzut Barbieri
<barbieri@profusion.mobi> wrote:
> On Mon, Apr 28, 2008 at 8:57 AM, Cedric BAIL <cedric.bail@free.fr> wrote:
>  > On Mon, Apr 28, 2008 at 4:31 AM, Gustavo Sverzut Barbieri
>  >  <barbieri@profusion.mobi> wrote:
>  >  >  The SoC student for python-eet (Luiz Irber) is doing the project even
>  >  >  without Google's money, so while reading the code in order to do
>  >  >  proper mentoring I found some minor details. If you're ok with them,
>  >  >  the student can provide patches for these and I can commit.
>  >
>  > >  3 - why not make EET_T_* and EET_G_* enums and use them in functions?
>  >  >  4 - in many places we use the construction:
>  >  >        len = strlen(str);
>  >  >        strcpy(copy, str);
>  >  >     why not use memcpy() since we already know the size?
>  >  >  5 - in other places it's bit worse:
>  >  >      copy = strdup(str);
>  >  >      len = strlen(str);
>  >  >    this should be replaced with (will save 1 walk of the whole string):
>  >  >      len = strlen(str);
>  >  >      copy = malloc(len + 1);
>  >  >      memcpy(copy, str, len + 1);
>  >
>  >  This should be part of a big refactor of eet code.
>
>  yes, but this don't need to be part of a big refactor, it can come in
>  small patches by GSoC students! :-)

Yep, it can come with small patch, but I would really like them to
came after 1.0.1.

>  >  >  6 - from 4 and 5 it also look that we should keep the string size
>  >  >  around and use it instead of these str* variants...
>  >
>  >  I already tracked all source of slow down inside eet_data due to str*,
>  >  i left the one that doesn't impact performance. But I am definitively
>  >  thinking about keeping the string size around, this should be also
>  >  part of a refactor.
>
>  I know keeping the size may not pay off sometimes, specially with
>  smaller strings. But again, one looking at it and checking where is
>  desirable to keep an integer with this is a good thing to do. Again, a
>  great job for students, we can help by reviewing the patches.

I personnaly don't care who write the code, well that's not true, if
someone else than me is doing it, and do it well, it's much better :-)

>  >  >  9 - minor weirdness in casts... not respecting const where it could
>  >  >  (eet_data_{get,put}_*), unneeded casts for malloc.
>  >  >  10 - some code paths could be split into functions and be reused, to name one:
>  >  >              if (data)
>  >  >                {
>  >  >                   echnk = eet_data_chunk_new(data, size, ede->name,
>  >  >  ede->type, ede->group_type);
>  >  >                   eet_data_chunk_put(ed, echnk, ds);
>  >  >                   eet_data_chunk_free(echnk);
>  >  >                   free(data);
>  >  >                   data = NULL;
>  >  >               }
>  >  >     this is replicated all over the code...
>  >
>  >  This should be also part of refactoring the code.
>
>  again, this can be a small patch for today's EET and done by our dear
>  students. As raster pointed out at IRC we may have just 5 of these,
>  but yet, a student can just move this to a function, test and provide
>  the patch... maybe it will bring us developers more work to explain
>  how to provide a good patch than to fix it ourselves, but at least it
>  will get more people involved in the code.

>  >  >  11 - split dump code from decode... at least move the code inside if
>  >  >  (dumpfunc) to another func, it will make code easier to read and still
>  >  >  might make it a bit fast, because most used functions will be smaller.
>  >
>  >  I was thinking about adding a way for some generic pretty printer, so
>  >  it would be easy to write another kind of output. Like a json output,
>  >  or maybe directly the javascript internal object for me. This kind of
>  >  improvement would also help writing support in other scripting langage
>  >  when you want to do eet->script objects.
>  >
>  >  So if people agree, I am thinking we can plan to :
>  >  - Add unit test and covering to eet with cutest
>  >  - Refactorise/clean code of eet_data
>  >  - Add array support to eet_data
>  >  - Add "generic" pretty printer for script binding (use it later to
>  >  provide undump) to eet_data
>  >  - Add optional support for symetric crypto support per section in
>  >  eet_lib (could be usefull for private config)
>  >
>  >  This could be the todo for version 2.0. I will not start working on
>  >  this until 1 or 2 months, so if others want to help. But first make a
>  >  1.0.1 with all small fix before we start breaking thing and add new
>  >  features.

>  yeah, I discussed this a bit at IRC (but you were away), the idea was
>  to split the decoder walker from the part that construct the data or
>  output the contents. In a simple implementation we can just provide a
>  callback to be called with type, group_type, data and size. Then even
>  the C implementation could be build on top of it, also the
>  text-dump... and of course Python or JavaScript or Ruby :-)

Yep, that was my idea too. It's good to know that someone else is
going to help on this. I will help reviewing code and answer questions
on this subject.

-- 
Cedric BAIL

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
[prev in list] [next in list] [prev in thread] [next in thread]