libtool-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [patch] fix nits in recent cwrapper patch


From: Eric Blake
Subject: Re: [patch] fix nits in recent cwrapper patch
Date: Fri, 8 Jun 2007 18:36:07 +0000 (UTC)
User-agent: Loom/3.14 (http://gmane.org/)

Charles Wilson <libtool <at> cwilson.fastmail.fm> writes:

> 
> Hopefully the attached addresses all of the concerns mentions by Noah, 
> Erick, and Peter with regards to my recent cwrapper-emits-shwrapper patch.

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 :)

> 2007-06-08  Charles Wilson  <...>
> 
>       (func_emit_libtool_cwrapperexe_source) [file scope]:
>       define permission flags S_IXGRP and S_IXOTH if not
>       already defined.
>       (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.

>       (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.

> 
> -#undef LTWRAPPER_DEBUGPRINTF
> +void LTWRAPPER_DEBUGPRINTF(const char* fmt, ...)

One last nit - why are we exporting this?  Make it look like this:

static void
LTWRAPPER_DEBUGPRINTF(const char* fmt, ...)


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.]

-- 
Eric Blake






reply via email to

[Prev in Thread] Current Thread [Next in Thread]