[OPEN-ILS-DEV] C nits: utils.h

[Scott McKellar]

In utils.h, the OSRF_MALLOC macro should be wrapped in a do-while-zero
loop, like the OSRF_BUFFER_ADD and OSRF_BUFFER_ADD_CHAR macros.

Otherwise, suppose someone codes something like:

   char * p;
   ...
   if( p = NULL )
      OSRF_MALLOC( p, sizeof( foo ) );

Once the macro is expanded, its first statement will be subject to the
"if" test, but the second and third will not -- they will execute
unconditionally.  That's almost certainly not what the coder intended,
nor what a reader would expect.

In general, any macro that includes multiple statements should be
wrapped in a do-while-zero loop.

THe VA_LIST_TO_STRING macro also includes multiple statements.
Unfortunately, a do-while-zero loop here would break the macro.  It
returns a value to the containing code, not by assigning to a macro
argument, but by assigning to a variable VA_BUF that is declared
within the macro itself.  If VA_BUF were declared inside a loop, it
would be invisible outside the loop, and hence useless.

Even if we could somehow make VA_BUF visible outside the loop -- by
declaring it at file scope, for example -- it points to an array
that also needs to be visible outside the loop.  Because the array
is of variable length, it has to be declared within the macro.

So, no loop -- and the macro is vulnerable to the sort of mishap
described above.

In addition, since this macro declares several variables, it invites
collisions with other variables with the same names in the same 
scope.  The attempted solution to that problem is to give the
variables funny names that are unlikely to appear elsewhere in the
containing code.

Even with the funny names, it is impossible to use the same macro 
twice in the same scope.  Granted, in this case it's unlikely that 
anyone would want to.

(As I have mentioned in an earlier note, one of the funny names is
reserved for the C implementation, because it starts with two
underscores.  A better way to make a funny name is to add trailing
underscores.)

A typical reason for defining a function-like macro is to avoid the
overhead of a function call for a trivial operation.  However the
processing within this macro is far from trivial.  The overhead of a
function call would be a drop in the bucket.  For that matter you 
would eliminate far more overhead by eliminating the call to bzero(), 
which in this context does nothing useful.

Another reason for defining a macro is to save typing.  Speaking only
for myself, I'd rather do the typing than rely on a macro that offers
so many modes of breakage.  (Besides, I like strongly typed 
languages.)

Similar considerations apply to the macros LONG_TO_STRING, 
DOUBLE_TO_STRING, LONG_DOUBLE_TO_STRING, and INT_TO_STRING.  Unlike 
the previous case, one might well want to use two of them in the
same scope, or the same one twice.  But one can't, because they all
declare some of the same variables.

Scott McKellar
http://home.swbell.net/mck9/aargh/