On Sun, 27 Apr 2008 23:31:27 -0300 "Gustavo Sverzut Barbieri"
<[EMAIL PROTECTED]> babbled:
> Hi,
>
> 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.
>
> 1 - minor spell errors in the docs, easy to fix using aspell/ispell
sure. not critical :)
> 2 - why descriptor macros don't use offsetof() macros (in GCC one can
> do it using __builtin_offsetof()). Vincent, can you confirm if similar
> stuff is available on Windows? This would improve readability.
because offsetof is a macro anyay - the contents of that macro is in effect the
same as in the EET_DATA_DESCRIPTOR_ADD* macros (except they just happen to
use temporary stack pointers instead of a 0 pointer and casting). the result is
the same and the eet macro method is portable, whereas the offsetof macro may
or may not be defined on your target. i see zero ue in changing the code for
the sake of changing it :)
> 3 - why not make EET_T_* and EET_G_* enums and use them in functions?
perfedctly possible - won't improve the code really in any useful way though :)
> 4 - in many places we use the construction:
> len = strlen(str);
> strcpy(copy, str);
> why not use memcpy() since we already know the size?
yes - we can. but in practice - you will not see any difference. they
essentially boil down to the same thing. memcpy() will copy something, compare
index position compared to size and if >= size, stop. the unrolling nature of
memcpy may be a little faster.. with long strings, but with short ones strcpy
will be as fast, or a bit faser as there is never a decision as to possibly
using mmx, sse or other optimised versions for used for larger chunks. in the
end - much of a muchness. the len = bit was used to make sure we allocate a
buffer of the right size so we have no overruns.
again - much of a muchness here. 6 or half-a-dozen. vanilla or chocolate. :)
> 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);
yes - that's better.
> 6 - from 4 and 5 it also look that we should keep the string size
> around and use it instead of these str* variants...
depends on the exact situation.
> 7 - why endianess/bytesex is detected at runtime?
because i was feeling too lazy to add the autofoo macros in. it's only
determined once @ runtime anyway (first use). much of a muchness really again.
> 8 - eet_coder is sometimes accessed with type and others with (type -
> 1), is this correct? It would be better to convert to the correct (0
> or 1 start) and go with it all along. This was a quick look, no real
> attempt to understand surrounding code.
i smell a bug here. i see 1. it should always be type -1 as the array starts @
0 but types start at 1 (type 0 is unknown and invalid). a list of sinple types
(basic types) allocates the wrong memory. it happens to work most of the time
as we almost never use this, and the types tend to go in order from smaller to
larger, thus almost always over-allocating (eg allocing a short instead of a
char, allocing an long long instead of an int, from long long -> float is goes
down, but i don't think a list of floats is ever used, as well as double -> char
(a list of chars is never used). so we have avoided this bug by just never
triggering it (as best i can see). fixed in cvs. (this maaaay explain some
mysterious bugs... i need to double check if we actually did trigger this or
not).
> 9 - minor weirdness in casts... not respecting const where it could
> (eet_data_{get,put}_*), unneeded casts for malloc.
possible - tho const vs non-const generally is just to shut the damn compiler
up. a pointer is a pointer.
> 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...
actually only 5 instances. not that many. sure - it can be a function. we could
spend a LOT of time factoring out code into functions in many places in efl -
this is the least of this kind of thing. you'll find much worse :P i'm happy for
someone to spend time doing this :)
> 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.
you likely are going to make the code larger - dumpfunc is something i didn't
WANT in eet. i fought against it forever. i did it to shut the complainers up
with "but we can't edit the files in a text editor". it's there. u can dump
into text, edit and convert back to eet data. does anyone actually use it( eet -
the cmd-line tool can do this)... NOO - of course not! people are mostly a
large lot of whiners who complain and when u actually solve their problems-
they never use the solution. why? because what u created to start with is good
anyway. so yes - this has ugliness, but it works. i'd be very careful factoring
this off somewhere else as it could easily break this.
> As you can see it's a bunch of _MINOR_ stuff, very good to get
> students used to E code and start contributing! If you're ok,
> students could provide one patch for each item, these would be posted
> to the ML and then we can commit.
sure! i agree - most minor.. EXCEPt the one bug above that concerned me. fixed
in cvs now.
btw. as of 1.0.0 of EFL i hope to keep a VERY detailed changelog. please - if
you have patches for eet (or anything that has hit 1.0.0) provide full
changelog patches too.
as i mentioend above - some of these are "much of a muchness" things that make
no practical difference and are shuffling sand from pile A to pile B. others
are slightly nicer stylistically, others may be dangerous. a lot of these are
just the result of code that evolves over time and once it works and works
well is not really polished to perfection as its functionality is all there and
solid and we do have other things to focus on.
i really do appreciate people who WANT to polish the code and have the time and
energy. thankfully eet is a tiny bit of code that is easily managable. you can
get your head around it and its design and intentions very easily. other libs
(like edje) get nasty to get to terms with - evas even more so. so polish on
eet is all fine.i need to do 1.0.1 now with a bugfix. i'll delay this a few
weeks to see if anything else crops up :)
> --
> Gustavo Sverzut Barbieri
> http://profusion.mobi
> Embedded Systems
> --------------------------------------
> MSN: [EMAIL PROTECTED]
> Skype: gsbarbieri
> Mobile: +55 (81) 9927 0010
>
> -------------------------------------------------------------------------
> 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
>
--
------------- Codito, ergo sum - "I code, therefore I am" --------------
The Rasterman (Carsten Haitzler) [EMAIL PROTECTED]
-------------------------------------------------------------------------
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
