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

[Corinna Vinschen <vinschen at redhat dot com>]

On Mar  5 23:36, Eric Blake wrote:
> Corinna Vinschen <vinschen <at> redhat.com> writes:
> > Index: libc/include/wchar.h
> > ===================================================================
> > +
> > +int	_EXFUN(fwprintf, (__FILE *, const wchar_t *, ...));
> > +int	_EXFUN(swprintf, (wchar_t *, size_t, const wchar_t *, ...));
> > +int	_EXFUN(vfwprintf, (__FILE *, const wchar_t *, __VALIST));
> > +int	_EXFUN(vswprintf, (wchar_t *, size_t, const wchar_t *, __VALIST));
> > +int	_EXFUN(vwprintf, (const wchar_t *, __VALIST));
> > +int	_EXFUN(wprintf, (const wchar_t *, ...));
> 
> Missing #ifndef _REENT_ONLY guarding around the non-reentrant versions?

None of the functions in wchar.h. is guarded that way.  I rather keep it
as it is, unless somebody does it for all of wchar.h.

> > +++ libc/stdio/Makefile.am	5 Mar 2009 18:01:19 -0000
> >  <at>  <at>  -132,7 +132,12  <at>  <at>  ELIX_4_SOURCES = \
> >  	putwchar.c		\
> >  	ungetwc.c		\
> >  	vasniprintf.c		\
> > -	vasnprintf.c
> > +	vasnprintf.c		\
> > +	vwprintf.c		\
> > +	swprintf.c		\
> > +	vswprintf.c		\
> > +	wprintf.c		\
> > +	fwprintf.c
> 
> Does it make sense to sort this list alphabetically?

Done.

> > diff -N libc/stdio/swprintf.c
> > +
> > +        <<swprintf>> is like <<wprintf>>, except that output is directed
> > +        to the buffer <[str]>, and the resulting string length is limited
> > +	to at most <[size]> bytes, including the terminating <<NUL>>.  As
> > +	a special case, if <[size]> is 0, <[str]> can be NULL, and
> > +	<<swprintf>> merely calculates how many bytes would be printed.
> 
> s/bytes/wide characters/

Fixed.

> > +RETURNS
> > +On success, <<swprintf>> return the number of bytes in
> 
> s/bytes/wide characters/

Fixed.

> > +the output string, except the concluding <<NUL>> is not counted.
> > +<<wprintf>> and <<fwprintf>> return the number of characters transmitted.
> > +
> > +If an error occurs, the result of <<wprintf>>, <<fwprintf>>, and
> > +<<swprintf>> is a negative value.  For <<wprintf>> and <<fwprintf>>,
> > +<<errno>> may be set according to <<fputc>>.  For <<snwprintf>>, <<errno>>
> 
> s/fputc/fputwc/

Fixed.

> > +may be set to EOVERFLOW if <[size]> or the output length exceeds
> > +INT_MAX / sizeof (wchar_t).
> > +
> > +PORTABILITY
> > +POSIX-1.2008
> > +
> > +#ifdef _HAVE_STDC
> > +#include <stdarg.h>
> > +#else
> > +#include <varargs.h>
> > +#endif
> 
> By this day and age, _HAVE_STDC should always be set, and we should be able to 
> blindly #include <stdarg.h> without worrying about the obsolete <varargs.h>.  
> Likewise for all futher uses of _HAVE_STDC in this file.

Fixed.

> > Index: libc/stdio/vfwprintf.c
> > ===================================================================
> > +	/* sorry, fwprintf(read_only_file, "") returns WEOF, not 0 */
> > +	if (cantwrite (data, fp)) {
> > +		_funlockfile (fp);
> > +		return (WEOF);
> 
> Are we guaranteed that WEOF is negative?  WEOF is of type wint_t, and it is 
> feasible (although I don't know of any systems that do this) that wint_t could 
> be unsigned, and that (int)WEOF is not negative.  Since *wprintf return int and 
> not wint_t, shouldn't this be an explicit -1 rather than WEOF?

Reverted to EOF.  We know that it's -1.

> > +#ifdef _MB_CAPABLE
> > +			if (ch == L's' && !(flags & LONGINT)) {
> 
> By definition, since this is wprintf, shouldn't _MB_CAPABLE be set?  In other 
> words, does it buy us anything to make this chunk conditional?

No.  A target could deliberately not support multibyte charsets, only
singlebyte but nevertheless support wide char functions.  That's why 
you see _MB_CAPABLE in a couple of wc functions.  It simplifies the
code extremly for small targets.

> > +#endif /* _MB_CAPABLE */
> > +			if (prec >= 0) {
> > +				/*
> > +				 * can't use strlen; can only look for the
> 
> s/strlen/wcslen/

Fixed.

> > +_CONST static CH_CLASS chclass[256] = {
> 
> Should this and the following state tables be moved into the __ namespace, 
> exported, and shared with the character vfprintf for space savings?

Good idea!  I did that in the patch which I applied.


Thanks for your review,
Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat