This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] vfprintf: Reuse work_buffer in group_number
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org
- Date: Tue, 27 Jun 2017 15:07:43 -0300
- Subject: Re: [PATCH] vfprintf: Reuse work_buffer in group_number
- Authentication-results: sourceware.org; auth=none
- References: <20170619162002.68BAE402AEC0E@oldenburg.str.redhat.com>
On 19/06/2017 13:20, Florian Weimer wrote:
> 2017-06-19 Florian Weimer <fweimer@redhat.com>
>
> * stdio-common/vfprintf.c (group_number): Add front_ptr argument.
> Use it to make the temporary copy at the start of the work buffer.
> (process_arg): Adjust call to group_number.
LGTM, thanks.
>
> diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
> index f0ea4fe..e0c6edf 100644
> --- a/stdio-common/vfprintf.c
> +++ b/stdio-common/vfprintf.c
> @@ -594,9 +594,8 @@ static const uint8_t jump_table[] =
> string = _itoa (number.longlong, workend, base, \
> spec == L_('X')); \
> if (group && grouping) \
> - string = group_number (string, workend, grouping, \
> - thousands_sep); \
> - \
> + string = group_number (work_buffer, string, workend, \
> + grouping, thousands_sep); \
> if (use_outdigits && base == 10) \
> string = _i18n_number_rewrite (string, workend, workend); \
> } \
> @@ -652,9 +651,8 @@ static const uint8_t jump_table[] =
> string = _itoa_word (number.word, workend, base, \
> spec == L_('X')); \
> if (group && grouping) \
> - string = group_number (string, workend, grouping, \
> - thousands_sep); \
> - \
> + string = group_number (work_buffer, string, workend, \
> + grouping, thousands_sep); \
> if (use_outdigits && base == 10) \
> string = _i18n_number_rewrite (string, workend, workend); \
> } \
> @@ -1236,8 +1234,8 @@ static int printf_unknown (FILE *, const struct printf_info *,
> const void *const *) __THROW;
>
> /* Group digits of number string. */
> -static CHAR_T *group_number (CHAR_T *, CHAR_T *, const char *, THOUSANDS_SEP_T)
> - __THROW internal_function;
> +static CHAR_T *group_number (CHAR_T *, CHAR_T *, CHAR_T *, const char *,
> + THOUSANDS_SEP_T);
>
> /* The function itself. */
> int
> @@ -2012,16 +2010,20 @@ printf_unknown (FILE *s, const struct printf_info *info,
> return done;
> }
>
> -/* Group the digits according to the grouping rules of the current locale.
> - The interpretation of GROUPING is as in `struct lconv' from <locale.h>. */
> +/* Group the digits from W to REAR_PTR according to the grouping rules
> + of the current locale. The interpretation of GROUPING is as in
> + `struct lconv' from <locale.h>. The grouped number extends from
> + the returned pointer until REAR_PTR. FRONT_PTR to W is used as a
> + scratch area. */
> static CHAR_T *
> -internal_function
> -group_number (CHAR_T *w, CHAR_T *rear_ptr, const char *grouping,
> - THOUSANDS_SEP_T thousands_sep)
> +group_number (CHAR_T *front_ptr, CHAR_T *w, CHAR_T *rear_ptr,
> + const char *grouping, THOUSANDS_SEP_T thousands_sep)
> {
> + /* Length of the current group. */
> int len;
> - CHAR_T *src, *s;
> #ifndef COMPILE_WPRINTF
> + /* Length of the separator (in wide mode, the separator is always a
> + single wide character). */
> int tlen = strlen (thousands_sep);
> #endif
>
> @@ -2034,26 +2036,34 @@ group_number (CHAR_T *w, CHAR_T *rear_ptr, const char *grouping,
> len = *grouping++;
>
> /* Copy existing string so that nothing gets overwritten. */
> - src = (CHAR_T *) alloca ((rear_ptr - w) * sizeof (CHAR_T));
> - s = (CHAR_T *) __mempcpy (src, w,
> - (rear_ptr - w) * sizeof (CHAR_T));
> + memmove (front_ptr, w, (rear_ptr - w) * sizeof (CHAR_T));
> + CHAR_T *s = front_ptr + (rear_ptr - w);
> +
> w = rear_ptr;
>
> /* Process all characters in the string. */
> - while (s > src)
> + while (s > front_ptr)
> {
> *--w = *--s;
>
> - if (--len == 0 && s > src)
> + if (--len == 0 && s > front_ptr)
> {
> /* A new group begins. */
> #ifdef COMPILE_WPRINTF
> - *--w = thousands_sep;
> + if (w != s)
> + *--w = thousands_sep;
> + else
> + /* Not enough room for the separator. */
> + goto copy_rest;
> #else
> int cnt = tlen;
> - do
> - *--w = thousands_sep[--cnt];
> - while (cnt > 0);
> + if (tlen < w - s)
> + do
> + *--w = thousands_sep[--cnt];
> + while (cnt > 0);
> + else
> + /* Not enough room for the separator. */
> + goto copy_rest;
> #endif
>
> if (*grouping == CHAR_MAX
> @@ -2062,17 +2072,16 @@ group_number (CHAR_T *w, CHAR_T *rear_ptr, const char *grouping,
> #endif
> )
> {
> - /* No further grouping to be done.
> - Copy the rest of the number. */
> - do
> - *--w = *--s;
> - while (s > src);
> + copy_rest:
> + /* No further grouping to be done. Copy the rest of the
> + number. */
> + memmove (w, s, (front_ptr -s) * sizeof (CHAR_T));
> break;
> }
> else if (*grouping != '\0')
> - /* The previous grouping repeats ad infinitum. */
> len = *grouping++;
> else
> + /* The previous grouping repeats ad infinitum. */
> len = grouping[-1];
> }
> }
>