The new patch is attached.
On Sat, Jul 18, 2009 at 1:57 PM, Gustavo Sverzut Barbieri
<barbi...@profusion.mobi> wrote:
> On Fri, Jul 17, 2009 at 6:55 PM, Andre Dieb<andre.mart...@ee.ufcg.edu.br>
> wrote:
> > Hello,
> >
> > This patch contains an initial implementation of logging domains for eina
> > error module. Please be aware that this a very rough version and there
> are
> > lots of things that must be improved.
> >
> > The patch is also with lots of fprintf's that I was using for debugging,
> > please ignore them. When we're ready to apply, I'll cleanup everything.
> >
> > For testing the new domain features, I added three unit tests and fixed
> > eina_rectangle test that was preventing me from building eina_error test.
> > You can also see examples of how logging domains will be used on these
> tests
> > (registering, logging, unregistering).
>
> Okay, couple of issues. One of them was already spotted by Sachiel,
> but other follows:
>
> Header:
>
> +typedef int Eina_Error_Domain_Index;
>
> this typedef is a bit too much.
>
> +struct _Eina_Error_Domain
> +{
> + EINA_INLIST;
>
> ouch, this is legacy and not being used! Remove it.
>
> + unsigned int level; /**< Max level to log */
> + const char *name; /**< Domain name */
> + const char *domain_str; /**< Formatted string with color to print */
> + const char *color;
> +
> + /* Private */
> + int deleted : 1; /**< Flags deletion of domain, a free slot */
>
> Eina_Bool, otherwise deleted is just the sign bit and deleted is (-1,
> 0), not (1, 0)
>
>
> +typedef struct _Eina_Error_Domain_Level_Pending
> Eina_Error_Domain_Level_Pending
> +struct _Eina_Error_Domain_Level_Pending
>
> These are private to eina_error.c
>
> +struct _Eina_Error_Domain_Level_Pending
> +{
> + EINA_INLIST;
> +
> + const char *name;
> + unsigned int level;
> +};
>
> change to the following and get the pointer arithmetic for free, also
> saving one pointer access and sizeof(char*) bytes.:
> +struct _Eina_Error_Domain_Level_Pending
> +{
> + EINA_INLIST;
> +
> + unsigned int level;
> char name[];
> +};
>
> +typedef void (*Eina_Error_Dom_Print_Cb)(Eina_Error_Domain_Index i,
> Eina_Error_Level level, const char *file,
> + const char *fnc, int line, const char *fmt, void *data,
> + va_list args);
> +
>
> and all other callbacks: domain index is useless, you should give it
> the pointer to domain! Imagine I want to implement my own logging in
> my program, how could I access the log domain? Other than that, domain
> index is pretty much useless.
>
>
> Code:
>
> +static Eina_Error_Domain **_error_domains = NULL;
>
> Sachiel spotted you're misusing the _error_domains by allocating
> sizeof(Eina_Error_Domain) instead of pointer. But I guess this should
> be really an array of domains and not an array of pointers. So just
> change that to Eina_Error_Domain *_error_domains. That's why you need
> "domain->deleted", otherwise just free and set to NULL. The other
> option is to remove "deleted" and keep this as a pointer, but I guess
> it's marginally worse.
>
> +static int _error_domains_count = 0;
>
> see eina_array, you need 2 counters, one for current size (count) and
> another for allocated size. Then you do not need to memset()/NULL-ify
> remaining slots. If you want to avoid useless walks in this array, you
> can keep an extra "free" count, that would say the number of free
> slots inside _error_domains_count. It would increase when you remove
> elements before last. If you remove last, just decrease
> error_domains_count. If want to bit super-correct, when you remove
> last you'd walk backwards checking for already freed slots and move
> that amount from _free to _count. Ie:
>
> [xxxxx###], count=5, size=8, free=0
> [xxx#x###], count=5, size=8, free=1
> [xxx#####], count=3, size=8, free=0
>
>
> +static int _pending_list_count = 0;
>
> remove, this is useless.
>
>
> + d = malloc(sizeof(char) * (color_len + name_len +
> strlen(EINA_COLOR_WHITE) + 1));
>
> oops, don't assume everybody uses dark terminals! You should use
> EINA_COLOR_NOTHING. And while compiler should optimize strlen() for
> that string, you can remove that with sizeof(), and it already
> accounts trailing \0. Same for memcpy(), just give it sizeof() and it
> will copy the null terminator.
>
> + d->color = malloc(sizeof(char)*(strlen(color)+1));
> + memcpy((char *)d->color, color, strlen(color));
>
> you can just strdup() here. And while most compilers would optimize
> strlen(), keeping a variable with that value would make your code
> easier to read.
>
> if you allocate all the strings in a single memory blob, it's even better.
>
>
> +static void
> +eina_error_domain_free(Eina_Error_Domain *d)
> +{
> + if (!d) return;
> +
> + if (d->name)
> + free((char *)d->name);
> + if (d->domain_str)
> + free((char *)d->domain_str);
> +
> + free(d);
> +}
>
> how about color? that's the benefit of using a single blob, just free
> the pointer that is the head, everything else goes aways
> automatically.
>
> + // Form name:level,name1:level1,name2:level2,...
> ...
> + p->level = strtol((char *)(end + 1), &tmp, 10);
> ...
> + start = tmp + 1;
>
> doc fail! :-) You need to strchr(tmp, ',') to find out next comma.
> Also add that doc (and other env vars!) to header.
>
> + EINA_ERROR_DOMAIN_GLOBAL = eina_error_domain_register(NULL, NULL);
> i guess it would be better to give real values to this function and
> actually make it complain on NULL pointers, also adding
> EINA_ARG_NONNULL() to header signature.
>
>
> + // Check if color is disabled
> + if ((tmp = getenv("EINA_ERROR_COLOR_DISABLE")))
> + if (atoi(tmp) == 1) _disable_color = 1;
> do this before everything else, so functions that check for
> _disable_color will always consider it correctly.
>
> +finish_register:
> + EINA_INLIST_FOREACH(_pending_list, pending)
> + {
> + if (!strcmp(pending->name, name))
> + {
> + fprintf(stderr, "Updating domain %s level from %d to %d\n",
> name, _error_domains[i]->level, pending->level);
> + _error_domains[i]->level = pending->level;
> + _pending_list = eina_inlist_remove(_pending_list,
> EINA_INLIST_GET(pending));
> + break;
> + }
> + }
>
> oops, leaked pending!
>
>
> +eina_error_dom_print_cb_stdout(Eina_Error_Domain_Index i,
> Eina_Error_Level level,
> + const char *file, const char *fnc, int line, const char
> *fmt,
> + __UNUSED__ void *data, va_list args)
> +{
> + Eina_Error_Domain *d;
> + d = _error_domains[i];
> +
> + if (!d) return;
> +
> + printf("%s\t%s%s:%d %s()%s ", (d->domain_str) ? d->domain_str :
> ((d->name) ? d->name : ""),
> + (!_disable_color) ? _colors[level] : "",
> + file, line, fnc,
> + (!_disable_color) ?
> _colors[EINA_ERROR_LEVEL_INFO] : "");
> + vprintf(fmt, args);
> +}
>
> just guarantee these strings will always be valid. And give this
> callback a const Eina_Error_Domain. The point of having domain_str and
> name is to avoid doing these checks during every print.
>
> +
> + if (getenv("EINA_ERROR_ABORT")) abort();
> +}
>
> this is bad and would make abort on any debug if its level is visible.
> Just check if level is greater than error/critical/whatever.
>
> All in all it looks good, just fix these points and I'll commit.
>
> --
> Gustavo Sverzut Barbieri
> http://profusion.mobi embedded systems
> --------------------------------------
> MSN: barbi...@gmail.com
> Skype: gsbarbieri
> Mobile: +55 (19) 9225-2202
>
--
André Dieb Martins
Embedded Systems and Pervasive Computing Lab (Embedded)
Electrical Engineering Department (DEE)
Center of Electrical Engineering and Informatics (CEEI)
Federal University of Campina Grande (UFCG)
Blog: http://genuinepulse.blogspot.com/
------------------------------------------------------------------------------
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
