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