Erik Faye-Lund <kusmabite@xxxxxxxxxxxxxx> writes:
> On Thu, Nov 26, 2009 at 1:59 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Erik Faye-Lund <kusmabite@xxxxxxxxxxxxxx> writes:
>>
>>> +void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
>>> {
>>> int len;
>>>
>>> if (!strbuf_avail(sb))
>>> strbuf_grow(sb, 64);
>>> len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
>>> if (len < 0)
>>> die("your vsnprintf is broken");
>>> if (len > strbuf_avail(sb)) {
>>> strbuf_grow(sb, len);
>>> len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
>>> if (len > strbuf_avail(sb)) {
>>> die("this should not happen, your snprintf is broken");
>>> }
>>
>> Hmm, I would have expected to see va_copy() somewhere in the patch text.
>> Is it safe to reuse ap like this in two separate invocations of
>> vsnprintf()?
>
> I think your expectation is well justified, this seems to be a
> portability-bug waiting to happen. Sorry for missing this prior to
> sending out - on Windows this is known to work, and this function is
> currently only used from the Windows implementation of syslog.
>
> How kosher is it to use va_copy in the git-core, considering that it's
> C99? A quick grep reveals only one occurrence of va_copy in the
> source, and that's in compat/winansi.c. Searching the history of next
> reveals that Alex Riesen (CC'd) already removed one occurrence
> (4bf5383), so I'm starting to get slightly scared it might not be OK.
We tend to avoid C99 features and it saved us in a few occasions. Recent
MSVC port revealed that we still had a handful of decl-after-statments but
luckily the necessary fix-ups were minimal because I have been reasonably
careful to reject patches that add it long before MSVC port happened.
> In practice it seems that something like the following works
> portably-enough for many applications, dunno if it's something we'll
> be happy with:
> #ifndef va_copy
> #define va_copy(a,b) ((a) = (b))
> #endif
Since an obvious implementation of va_list would be to make it a pointer
into the stack frame, doing the above would work on many systems. On
esoteric systems that needs something different (e.g. where va_list is
implemented as a size-1 array of pointers, va_copy(a,b) needs to be an
assignment (*(a) = *(b))), people can add compatibility macro later.
Historically some systems that do have a suitable implementation had it
under the name __va_copy() instead, so it would have been better to define
it as something like:
#ifndef va_copy
# ifdef __va_copy
# define va_copy(a,b) __va_copy(a,b)
# else
# /* fallback for the most obvious implementation of va_list */
# define va_copy(a,b) ((a) = (b))
# endif
#endif
But I do not know it still matters in practice anymore.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html