Re: [patch] fix nits in recent cwrapper patch

[Charles Wilson]

Eric Blake wrote:
> s/Erick/Eric/, but that's okay (I've seen worse butchering of my name, as short 
> as it is, and I'm sure I've done worse to those with longer names :)

Sorry...

> >         (func_emit_libtool_cwrapperexe_source) [LTWRAPPER_DEBUGPRINTF]:
> >         declare as a function, not a macro, as variadic macros
> >         are not supported by C89.
>
> This means that all arguments to LTWRAPPER_DEBUGPRINTF are now evaluated, even 
> when before in a non-debug build they were not.  But I don't think that's a 
> show-stopper, though, as long as we aren't doing something stupid in 
> calculating those arguments.  And a quick glance didn't turn anything up.

Hmm.  Good point.  I think it is better to be safe than sorry...see below.

> >         (func_emit_libtool_cwrapperexe_source) [check_executable]:
> >         avoid embedded #ifdefs; use S_IXGRP and S_IXOTH
> >         unconditionally.
> >         (func_emit_libtool_cwrapperexe_source) [make_executable]:
> >         ditto.
>
> Looked right to me.

Ack.

> One last nit - why are we exporting this?  Make it look like this:
>
> static void
> LTWRAPPER_DEBUGPRINTF(const char* fmt, ...)

okay...but again, see below.

> and then I'm okay with your resolution to the nits that I brought up.
>
> [As far as I know, the only way in C89 to get variadic macro behavior that can 
> be entirely elided is with stuff like
>
> #if DEBUG
> # define WRAPPER(args) mywrapper args
> int mywrapper(char*format, ...);
> #else
> # define WRAPPER(args)
> #endif
> WRAPPER((arg1, arg2))
>
> but that means changing all callers, whereas simply trading C99 variadic macro 
> for a C89 variadic function only changes the definition, at the expense of the 
> semantic change of always evaluating the arguments.]

I remember when I originally adapted run.exe from xemacs for general use 
on cygwin, I had something like this.

I don't mind changing all the callers (there's only 550 lines total in 
the C wrapper anyway, as Peter pointed out).  And, I think it's better 
to be safe, in case some variable is (in some hypothetical later 
modification) declared only when DEBUGWRAPPER, and is then used in a 
LTWRAPPER_DEBUGPRINTF() statement.


How about this:

#undef LTWRAPPER_DEBUGPRINTF
#if defined DEBUGWRAPPER
# define LTWRAPPER_DEBUGPRINTF(args) ltwrapper_debugprintf args
static void
ltwrapper_debugprintf (const char *fmt, ...)
{
    va_list args;
    va_start (args, fmt);
    (void) vfprintf (stderr, fmt, args);
    va_end (args);
}
#else
# define LTWRAPPER_DEBUGPRINTF(args)
#endif

with appropriate changes to all callers of LTWRAPPER_DEBUGPRINTF?

--
Chuck