maedoc / libtvb

TVB C library
6 stars 8 forks source link

Convert log & err macros to functions #88

Open maedoc opened 8 years ago

maedoc commented 8 years ago

Currently logging handlers defined using macros along the lines of

typedef int (*printf_t)(char *fmt, ...);

/* global variable */
extern printf_t logger;

#define logmsg(fmt, ...) logger("[log] " fmt "\n", __VA_ARGS__)

but it would be if logmsg where a real function which prints all its args into a string and then passes it off to the designated handler,

int logmsg(char *fmt, ...)
{
  /* build char* msg */
  logger(msg);
}

This should use vsnprintf, c.f.

paramhanji commented 8 years ago

Is there a reason for having an int return type?

maedoc commented 8 years ago

Yes: originally, the macros are just calls to the user-chosen log handler (some printf implementation) which returns int, the number of characters written. The printf implementation was responsible for filling in the format string with its arguments.

This isssue is to avoid calling a printf function by formatting the string with the logging arguments and then passing that string to the user chosen log handler, which means it can be much simpler.

The reason this is useful is that when we use sddekit from Python, for example, we don't want to call libc's printf but actually we want to send our log message to Python's sys.stdout or maybe even use the logging module. As a second example, in MATLAB, if your C code calls printf and not MATLAB's mexPrintf, your message won't even appear in the MATLAB window

paramhanji commented 8 years ago

Hmm makes sense. If I understood right, the new function definitions should do away with sd_log.c completely and provide a layer of abstraction so that it can be used by python and Matlab. Is that right?

maedoc commented 8 years ago

Currently sd_log.c provides way of setting verbose, quiet flags as well as setting the current handler (sd_log_msg). These will stay. The following changes are required

So there's a little more to it than just deleting sd_log.c, do you see what I mean?

paramhanji commented 8 years ago

Yeah there seems to be more to it. Just a tiny(even elementary) doubt. Where would the default handler be present? In sddekit_api or somewhere else?

maedoc commented 8 years ago

for completeness I would declare the default handle in api header like

typedef void(*sd_log_handler)(char *);
void sd_log_handler_printf(char*);

then in sd_log.c

static sd_log_handler handler = &sd_log_handler_printf; // hanlder currently in use defaults to printf

void sd_log_handler_printf(char *msg) { printf("%s", msg); }

sd_log_msg(...) {
   char msg[2048];
   vsnprintf(msg, ...);
   (*handler)(msg); // this is calling the static var declared above
}

does this make sense?

paramhanji commented 8 years ago

Okay I implemented most of the checklist you provided. I'm just a little confused about the getters and setters bit.

maedoc commented 8 years ago

the get and set let the user provide a custom handler. In sddekit_api.h,

typedef void(*sd_log_handler)(char *);

SD_API void
sd_log_set_handler(sd_log_handler);

SD_API sd_log_handler
sd_log_get_handler();

and in sd_log.c

/* handler in use, defaults to printf handler */
static sd_log_handler handler = sd_log_handler_printf;

void sd_log_set_handler(sd_log_handler h)
{
    if ((handler = h) == NULL)
        sd_err('NULL handler provided');
}

sd_log_handler sd_log_get_handler() { return handler; }

Else where in sd_log.c, the static handler instance is called by sd_log_msg.