Re: [PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 26, 2009, 18:46 UTC
Message-ID: <7vbpip86q5.fsf@alter.siamese.dyndns.org>
In-Reply-To: <40aa078e0911260238rd0c90cag126709d1de5f50de@mail.gmail.com>

Erik Faye-Lund <kusmabite@googlemail.com> writes:

Show 33 quoted lines

> On Thu, Nov 26, 2009 at 1:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Erik Faye-Lund <kusmabite@googlemail.com> 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.

Show 6 quoted lines

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