Re: [Freeciv-Dev] freelog and LOG_DEBUG

[David Pfitzner <dwp@xxxxxxxxxxxxxx>]

Markus Linnala wrote:

> David Pfitzner <dwp@xxxxxxxxxxxxxx> writes:
> 
> > Or a more sophisticated system where one can turn on or off
> > debugs based on __FILE__. Some sort of macro system to avoid
> > excessive unnecessary function calls may be appropriate for
> > efficiency (like your patch).
> 
> How about this:

> I think this system is powerful enough (with some general shell
> magic) to suit all practical needs. 

Yes, looks good.

I am a bit concerned about how much stuff you put in
log.h rather than in a .c file.  I assume that some
of this is intentional; eg, every file which includes log.h
will get its own static version of freelog_debug_check_ok(),
which I assume is intentional so that hopefully gcc will
inline it and get rid of it for levels other than LOG_DEBUG

But I suspect it would be possible and preferable to do
this some other way, eg with a macro which does the check
vs DEBUG and LOG_DEBUG and then calls an extern function
if necessary.  And probably more likely to optimise away in
the right circumstances?

Having:

> +int log_files_cnt;
> +t_log_files **log_files;

in log.h seems wrong to me.  They should be "extern" in the
.h file and declared in some .c file (presumably log.c)

Now for some other ideas for further refinements:

We shouldn't have to look through the list of file
(in your freelog_debug_check_ok()) for every LOG_DEBUG
output.  Instead, each file which includes log.h should
have some static vars (declared via log.h) which store
whether there is LOG_DEBUG debugging for this file,
and what lines if specified.  And an "initialised" var
to call some (extern) function (cf freelog_debug_check_ok) 
for the first time.  And appropriate macros to check if
we have initialised, initialise if necessary, then 
use the static vars to see if we need to call freelog.

Actually, it might be nice to be able to change the debug
details from the server prompt.  So instead of an "initialised"
var maybe have a static counter which compares against an 
extern counter in civserver.c which is updated each time 
we change the logging details.

Regarding the problem of having macros with variadic parameters,
it looks like the option you've taken is to simply use functions 
when we don't have GCC and let people without GCC take the 
performance hit.  Which is quite ok by me.  

But another option to consider would be to have a macro with a 
fixed number of args where one of the args is the format string 
and all related variadic parameters enclosed in brackets.
Then the macro call looks a bit strange (with extra brackets)
but I think everything works; eg:

#define freelog(level, args) \
  do { if( ugly_stuff(level) ) { real_freelog args ; } } while(0)

called as, eg:

freelog(LOG_DEBUG, ("%s!!", foo));

(Or if that doesn't work exactly, there is _some_ way of doing
the above trick so that it does work :-)

Except I'm not sure how to get the level arg both sent to
real_freelog and used for ugly_stuff, so maybe that is no good.  
(Above ugly_stuff is probably not really a function, but macro code 
which checks initialisation of static vars and checks DEBUG etc.)

Incidently if freelog is to be done using macros etc, I assume 
it would be reasonable to also do the comparison between the 
level arg in the call and the global log_level in the macro.  
(Ie, in ugly_stuff).

Suggestion: can we have filename matching use strstr(), so that 
one doesn't have to use the full filename?  (Could give some
surprises if abbreviate too mcuh, but presumably can then 
use full filenames if want to be specific.)

Minor nits:
- please use fc_malloc() for mallocs (recent sources)
- I am not aware of any good reason to cast the return value
  from malloc.  (This is C, not C++.)

Regards,
-- David