This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 1/2] Add strfrom{d,f,l} and wcsfrom{d,f,l} functions
On Tue, 23 Aug 2016, Gabriel F. T. Gomes wrote:
> diff --git a/NEWS b/NEWS
> index fdcd7e7..bf5a307 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -7,6 +7,10 @@ using `glibc' in the "product" field.
>
> Version 2.25
>
> +* The functions strfromd, strfromf, and strfroml, as well as their
> + wide-character counterparts are present in libc. These functions are defined
> + in ISO/IEC TS 18661-1:2014 and convert a floating-point number into string.
I think the NEWS entry should be consecutive with other NEWS entries for
features from TS 18661-1, not at the top, and it also needs to mention the
*_l functions added as GNU extensions. You also need to say that the
wide-character versions are GNU extensions, not standard.
> +extern int __strfromf_internal (char * __dest, size_t __size,
> + const char * __format, float __f, int __group)
> + __THROW __nonnull ((3));
Why do you need these functions at all? Grouping should never be involved
at all, since there is no way to specify it with strfrom.
I'd expect: __strfromf_l (internal name, used in implementing strfromf);
strfromf_l (weak alias); strfromf (which calls __strfromf_l). No
_internal functions, no __strfromf because nothing else in libc calls it
and you only need __* names when there are other calls and namespace
issues arise. hidden_proto/hidden_def only for __strfromf_l because it's
the only one that gets called from within libc. And then the same for
other types and for wide-character functions. That's all.
> @@ -172,6 +212,7 @@ extern int __vfwprintf (__FILE *__restrict __s,
> const wchar_t *__restrict __format,
> __gnuc_va_list __arg)
> /* __attribute__ ((__format__ (__wprintf__, 2, 0))) */;
> +extern int __swprintf (wchar_t *s, size_t n, const wchar_t *format, ...);
I don't see any calls to this function in this patch, so it shouldn't be
adding a prototype for it.
> +@node Printing of Floats
> +@section Printing of Floats
> +
> +@pindex stdlib.h
> +@pindex wchar.h
> +The @samp{str} functions are declared in @file{stdlib.h} and those
> +beginning with @samp{wcs} are declared in @file{wchar.h}.
> +
> +@comment stdlib.h
> +@comment ISO/IEC TS 18661-1
> +@deftypefun int strfromd (char *restrict @var{string}, size_t @var{size}, const char *restrict @var{format}, double @var{value})
I think all three functions should be covered here (with @deftypefunx for
the float and long double ones) rather than separating out the float and
long double ones (the only reason for them to be separated out for strtod
is probably that strtof and strtold were added in C99). Or maybe six or
twelve functions all together. In any case, the *_l functions are missing
documentation in this patch. And the "@comment ISO/IEC TS 18661-1" on the
wcs* functions is wrong, since they are GNU extensions.
> diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h
> index f0dc951..399e3b2 100644
> --- a/stdlib/stdlib.h
> +++ b/stdlib/stdlib.h
> @@ -21,6 +21,9 @@
>
> #ifndef _STDLIB_H
>
> +#define __GLIBC_INTERNAL_STARTING_HEADER_IMPLEMENTATION
> +#include <bits/libc-header-start.h>
> +
> #include <features.h>
This replaces an include of <features.h>, rather than going before it.
> @@ -230,6 +249,20 @@ extern long double strtold_l (const char *__restrict __nptr,
> char **__restrict __endptr,
> __locale_t __loc)
> __THROW __nonnull ((1, 3));
> +
> +# if __GLIBC_USE (IEC_60559_BFP_EXT)
> +extern int strfromd_l (char * __dest, size_t __size, const char * __format,
> + double __f, __locale_t __loc)
> + __THROW __nonnull ((3));
> +
> +extern int strfromf_l (char * __dest, size_t __size, const char * __format,
> + float __f, __locale_t __loc)
> + __THROW __nonnull ((3));
> +
> +extern int strfroml_l (char * __dest, size_t __size, const char * __format,
> + long double __f, __locale_t __loc)
> + __THROW __nonnull ((3));
> +# endif
This is inside __USE_GNU, which implies __GLIBC_USE (IEC_60559_BFP_EXT),
so don't put any conditional here.
> +/* Prototype for the internal implementation. */
> +#include <xlocale.h>
> +extern int INTERNAL (STRFROMF_L) (
> + STRING_TYPE *, size_t, const char *, FLOAT, int, __locale_t);
Parenthesis at end of line is not GNU style. Please fix throughout the
patch series.
> +INTERNAL (STRFROMF) (STRING_TYPE *dest, size_t size, const char * format,
No space after '*' in pointer declarations like this.
> + return INTERNAL(STRFROMF_L) (dest, size, format, f, group, _NL_CURRENT_LOCALE);
Missing space before '('. Please fix throughout this series.
> + /* Check if the format string is not null, even though TS-18661-1 does not
> + mention that the format string cannot be null. */
> + if (format == NULL)
> + {
> + MAYBE_SET_EINVAL;
> + return -1;
> + }
No. We don't include random checks for NULL like that. Standard glibc
practice is that it's appropriate for such undefined behavior to crash the
program, so just dereference the format string without checking.
> + /* According to ISO/IEC 9899:201x, Section 7.21.6.1, 5th paragraph, a
> + negative precision is taken as if the precision were omitted. Thus,
> + parse a potential minus sign. If the minus sign is present, ignore
> + the following precision digits. */
> + if (*format == '-')
No, that's wrong. A negative precision specified with '*' is ignored. A
precision in the string must be a decimal integer.
Also: throughout this code, you should be comparing with wide character
such as L'%' if using a wide string; see how the printf code uses a macro
L_. I advise using read_int from printf-parse.h (and note that it uses a
macro ISDIGIT not the function), not hardcoded reading of a precision
here.
> + /* If only the period is specified, the precision is taken as zero, as
> + described in ISO/IEC 9899:201x, section 7.21.6.1, 4th paragraph, 3rd
> + item. */
It's not 201x. It's 2011.
> + else
> + {
> + precision = 0;
> + }
No redundant braces around single statements. Please fix anywhere else in
the patch series as well.
> + /* The following code to prepare the virtual file has been adapted from the
> + functions _IO_vsnprintf and _IO_vswprintf from libio. */
> +
> + if (size == 0)
> + {
> +#ifdef USE_WIDE_CHAR
> + /* ISO/IEC 9899:201x, Section 7.29.2.3, in the third paragraph, specifies
> + that swprintf returns a negative value if n or more characters would
> + have been written. Thus, if size (n) is zero, return a negative number,
> + so that wcsfrom functions behave similar to swprintf. */
> + return -1;
No, this legacy peculiarity of how swprintf was originally specified in
C90 Amendment 1, before snprintf was added, should not be replicated.
Wide-character strfrom should act just like narrow-character strfrom as
regards the return value, and the documentation should make that clear.
(If you look at the Linux snprintf manpage, you'll see a discussion of how
pre-C99 versions of snprintf did things like swprintf, then this was
changed.)
> +#else
> + /* When size is zero, nothing is written and dest may be a null pointer.
> + This is specified for snprintf in ISO/IEC 9899:201x, Section 7.21.6.5,
> + in the second paragraph. Thus, if size is zero, prepare to use the
> + overflow buffer right from the start. */
Again, 2011, not 201x.
> + /* Terminate the string. */
> +#ifdef USE_WIDE_CHAR
> + if (sfile.f._sbf._f._wide_data->_IO_buf_base == sfile.overflow_buf)
> + /* ISO C99 requires swprintf to return an error if the output does not fit
> + in the provided buffer. The wcsfrom functions mimic this behavior. */
> + return -1;
Again, don't replicate legacy behavior; snprintf semantics are preferred
to those of swprintf.
> +/* Convert the floating-point value f, in `double' representation, to the wide
> + string `dest'. */
> +#if __GLIBC_USE (IEC_60559_BFP_EXT)
> +extern int wcsfromd (wchar_t * __dest, size_t __size, const char * __format,
> + double __f) __THROW __nonnull ((3));
> +#endif
> __END_NAMESPACE_STD
>
> #ifdef __USE_ISOC99
> @@ -462,6 +468,12 @@ extern float wcstof (const wchar_t *__restrict __nptr,
> wchar_t **__restrict __endptr) __THROW;
> extern long double wcstold (const wchar_t *__restrict __nptr,
> wchar_t **__restrict __endptr) __THROW;
> +#if __GLIBC_USE (IEC_60559_BFP_EXT)
> +extern int wcsfromf (wchar_t * __dest, size_t __size, const char * __format,
> + float __f) __THROW __nonnull ((3));
> +extern int wcsfroml (wchar_t * __dest, size_t __size, const char * __format,
> + long double __f) __THROW __nonnull ((3));
> +#endif
These conditions are wrong. These functions are GNU extensions, so you
need to use #ifdef __USE_GNU.
--
Joseph S. Myers
joseph@codesourcery.com