On Fri, Jul 17, 2009 at 6:55 PM, Andre Dieb<[email protected]> 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: [email protected]
Skype: gsbarbieri
Mobile: +55 (19) 9225-2202
------------------------------------------------------------------------------
Enter the BlackBerry Developer Challenge
This is your chance to win up to $100,000 in prizes! For a limited time,
vendors submitting new applications to BlackBerry App World(TM) will have
the opportunity to enter the BlackBerry Developer Challenge. See full prize
details at: http://p.sf.net/sfu/Challenge
_______________________________________________
enlightenment-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
