On 7/19/05, Greg Hudson <ghudson@mit.edu> wrote:
>
> On Tue, 2005-07-19 at 20:08 -0700, Brian Holmes wrote:
> > #define svn_repos_write_errorlog(repos, log_level, message) \
> > do { \
> > if ((repos)->error_log->level <= log_level) \
> > svn_log_write((repos)->error_log, log_level, message) \
> > } while(0);
> >
> > The value of the macro here is huge. The macro allows us to:
> > * enforce a constant method of logging that checks the log level
> > before calling the function
> > * prevents us from processing the 'message' parameter when
> > unnecessary.
> > * makes our code much more readable.
> > * makes logging simple from a programmer perspective while keeping
> > things efficient.
>
> You've managed to say two things in four ways, and one of them isn't
> true. Because the macro sacrifices varargs, our code looks less
> readable:
>
> SVN_REPOS_LOG(repos, log_level, apr_psprintf(pool, fmt, args), pool)
>
> is less readable than
>
> svn_repos_log(repos, log_level, pool, fmt, args)
>
> Your macro also requires us to expose the internals of svn_repos_t in
> the ABI; svn_repos_t is currently an opaque type.
Yeah, I stretched that one a bit. :) I was not aware that svn_repos_t was
opaque however, and that does change my view on things.
> * we can easily make changes to logic by changing the macro (like
> > checking if repos or repos->error_log is null before calling the
> > function) or we could completely (and possibly selectively) remove the
> > logging code altogether.
>
> We can easily make changes to the logic of the function too. Macros are
> not superior to functions as a means of abstraction.
They are superior only in the sense that we can put logic before the
function call and thus "save a few cycles."
> I'm not sure what to say about the var args. It is true that using a
> > macro will not allow us to use var args cleanly. However, do we
> > really need to use varargs and does it outweight my purported
> > benefits?
>
> For the kind of logging we will actually be adding (and so far, no one
> has had a *single* counterexample to this), the purported benefits allow
> us to save a few cycles before executing an operation such as "checkout"
> which will take millions upon millions of cycles to execute.
True that svn_repos may not use the logging extensively, at least not for
basic access or error messages, but if we started using it for debug tracing
it could add up. I was making the assumption that we might actually say
svn_repos_log(repos, DEBUG_TRACE, pool, fmt, args);
at which point if they were in low level functions being called hundreds of
thousands of times they would certainly chew up a lot of time.
Compared to that, the readability of a varargs function is orders of
> magnitude more important.
>
>
With your example, I have to agree that varargs is much easier to read.
Given the varargs debate, and more importantly, the opaqueness of
svn_repos_t, it seems prudent to use a function rather than a macro.
- Brian
Received on Wed Jul 20 23:06:38 2005