Re: [PATCH] New functions wprintf, fwprintf, swprintf, vwprintf, vfwprintf, vswprintf

[Corinna Vinschen <vinschen at redhat dot com>]

On Mar  9 12:14, Howland Craig D (Craig) wrote:
> 1)  There's a comment in vfwprintf.c (copied verbatim from vfprintf.c,
> so this is nothing new) regarding the POSIX "'" flag (an extension to
> C99):
> 
> /* The ' flag is required by POSIX, but not C99.
>    In the C locale, LC_NUMERIC requires
>    thousands_sep to be the empty string.  And since
>    no other locales are supported (yet), this flag
>    is currently a no-op.  */
>  
> Is the statement regarding no other locales still true given the recent
> updates to the locale stuff?  That is, could this ' flag be made
> operable now?

It could be done regardless of the state of the locale code.  The locale
code doesn't support any locale besides C yet, but the interfaces are
already available.  I don't think anything speaks against using
_localeconv_r (data)->thousands_sep right now.  Just keep in mind that
the result can be an empty string as well as a multibyte string.

> 2)  In the string form of the functions, there's an odd return check,
> which has also been copied from the non-wide starting-point code.  For
> example, lines 583-586 of swprintf.c:
>  
>   ret = _svfwprintf_r (ptr, &f, fmt, ap);
>   va_end (ap);
>   if (ret < EOF)
>     ptr->_errno = EOVERFLOW;
>  
> As far as I could see from a brief look through _svfwprintf_r(), it
> cannot
> return anything < EOF.  (Nor can _svfprintf_r().)  The EOVERFLOW return
> for the conditions described in the docs is handled further up, before
> _svfwprintf_r() is called.  Does anyone know what the real purpose of
> this check is?  (It is not a simple typo for "if(ret < 0)" assuming that
> the only error return from _svfwprintf_r() is that it ran out of space,
> as _svfwprintf_r() does not return an error if it ran out of space in
> the string.  Rather, it returns how many characters would have been put
> into the string while only putting in one less than the max, allowing
> room for '\0' to be appended.)
>      I think that it should be modified to be:
> 
>   ret = _svfwprintf_r (ptr, &f, fmt, ap);
>   va_end (ap);
>   if (ret >= size)
>     ptr->_errno = EOVERFLOW;
>  
> (I presently have done this in my local testing copy, along with the
> other changes that I have, but wanted to get a second opinion since the
> original if does not make sense to me.)

Jeff added this check in March 2007 but I don't see any code which
could result in a return value < EOF either.  Jeff?

> 3)  The documentation says only POSIX-1.2008.  Was C99 purposely left
> out?
> I'd think that C99 should be mentioned, and the POSIX extensions to C99
> highlighted in the documentation.  (If there's agreement that this
> approach makes sense, I'll do so when I send patches later for the bug
> fixes that I have so far.)

Personally I don't care for C99, I'm only looking for POSIX compatibility.
Feel free to extend the documentation to mention C99.

> 4)  Should the POSIX extensions be conditionally able to be left out?

No.  Coding conditionalized should be left to the application
programmer, the lib should support all cases.

> FYI, the aforementioned problems that I found and have developed fixes
> for are that the string forms do not correctly put the terminating '\0'
> in (at least not when sizeof(wchar_t) > 1), and that the string forms
> are supposed to return an error if size is exceeded.  (The POSIX man
> page fails to mention this part of the C99 standard, but it is still a
> requirement since POSIX explicitly defers to C99.)  The former fault
> is probably due to unclear coding in vfprintf.c, so that when porting to
> wide it was not obvious that a wide change was needed.  The latter is
> due
> to the oversight (error) in the POSIX manual.

I'm looking forward to the patch.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat