Re: [PATCH] maint: add more control flags to mbsalign

[Jim Meyering]

Pádraig Brady wrote:
> In support of the upcoming `df --output` and
> $cmd --help auto alignment changes, the attached
> patch gives more formatting control to callers of mbsalign().

Thanks.  I've tested that, and it passed.

> Subject: [PATCH] maint: add more control flags to mbsalign
>
> * gl/lib/mbsalign.h: Add MBA_UNIBYTE_ONLY (to allow
> faster processing), and MBA_NO_{LEFT,RIGHT}_PAD

It's slightly better to spell out those symbol names,
to ease log searching.

> to give greater control of padding, useful with the first
> or last fields on a line.
> * gl/lib/mbsalign.c (mbsalign): Implement the new flags.
> * gl/tests/test-mbsalign.c (main): Test combinations
> of the new flags.
> ---
>  gl/lib/mbsalign.c        |   63 ++++++++++++++++++++++++++-------------------
>  gl/lib/mbsalign.h        |   27 ++++++++++++++-----
>  gl/tests/test-mbsalign.c |   32 +++++++++++++++++++++++
>  3 files changed, 88 insertions(+), 34 deletions(-)
>
> diff --git a/gl/lib/mbsalign.c b/gl/lib/mbsalign.c
> index e45456b..3c3170a 100644
> --- a/gl/lib/mbsalign.c
> +++ b/gl/lib/mbsalign.c
> @@ -126,7 +126,7 @@ mbsalign (const char *src, char *dest, size_t dest_size,
>    /* In multi-byte locales convert to wide characters
>       to allow easy truncation. Also determine number
>       of screen columns used.  */
> -  if (MB_CUR_MAX > 1)
> +  if (!(flags & MBA_UNIBYTE_ONLY) && MB_CUR_MAX > 1)
>      {
>        size_t src_chars = mbstowcs (NULL, src, 0);
>        if (src_chars == SIZE_MAX)
> @@ -191,37 +191,46 @@ mbsalign_unibyte:
>    /* indicate to caller how many cells needed (not including padding).  */
>    *width = n_cols;
>
> -  /* indicate to caller how many bytes needed (not including NUL).  */
> -  ret = n_used_bytes + (n_spaces * 1);
> +  {
> +    size_t start_spaces, end_spaces;
>
> -  /* Write as much NUL terminated output to DEST as possible.  */
> -  if (dest_size != 0)
> -    {
> -      size_t start_spaces, end_spaces, space_left;
> -      char *dest_end = dest + dest_size - 1;
> +    switch (align)
> +      {
> +      case MBS_ALIGN_LEFT:
> +        start_spaces = 0;
> +        end_spaces = n_spaces;
> +        break;
> +      case MBS_ALIGN_RIGHT:
> +        start_spaces = n_spaces;
> +        end_spaces = 0;
> +        break;
> +      case MBS_ALIGN_CENTER:
> +      default:
> +        start_spaces = n_spaces / 2 + n_spaces % 2;
> +        end_spaces = n_spaces / 2;
> +        break;
> +      }
> +
> +      if (flags & MBA_NO_LEFT_PAD)
> +        start_spaces = 0;
> +      if (flags & MBA_NO_RIGHT_PAD)
> +        end_spaces = 0;
>
> -      switch (align)
> +      /* Write as much NUL terminated output to DEST as possible.  */
> +      if (dest_size != 0)
>          {
> -        case MBS_ALIGN_LEFT:
> -          start_spaces = 0;
> -          end_spaces = n_spaces;
> -          break;
> -        case MBS_ALIGN_RIGHT:
> -          start_spaces = n_spaces;
> -          end_spaces = 0;
> -          break;
> -        case MBS_ALIGN_CENTER:
> -        default:
> -          start_spaces = n_spaces / 2 + n_spaces % 2;
> -          end_spaces = n_spaces / 2;
> -          break;
> +          size_t space_left;
> +          char *dest_end = dest + dest_size - 1;
> +
> +          dest = mbs_align_pad (dest, dest_end, start_spaces);
> +          space_left = dest_end - dest;
> +          dest = mempcpy (dest, str_to_print, MIN (n_used_bytes, 
> space_left));
> +          mbs_align_pad (dest, dest_end, end_spaces);
>          }
>
> -      dest = mbs_align_pad (dest, dest_end, start_spaces);
> -      space_left = dest_end - dest;
> -      dest = mempcpy (dest, str_to_print, MIN (n_used_bytes, space_left));
> -      mbs_align_pad (dest, dest_end, end_spaces);
> -    }
> +    /* indicate to caller how many bytes needed (not including NUL).  */
> +    ret = n_used_bytes + ((start_spaces + end_spaces) * 1);
> +  }
>
>  mbsalign_cleanup:
>
> diff --git a/gl/lib/mbsalign.h b/gl/lib/mbsalign.h
> index e9340f9..d0eeb14 100644
> --- a/gl/lib/mbsalign.h
> +++ b/gl/lib/mbsalign.h
> @@ -21,20 +21,33 @@ typedef enum { MBS_ALIGN_LEFT, MBS_ALIGN_RIGHT, 
> MBS_ALIGN_CENTER } mbs_align_t;
>  enum {
>    /* Use unibyte mode for invalid multibyte strings
>       or when heap memory is exhausted.  */
> -  MBA_UNIBYTE_FALLBACK = 0x0001
> +  MBA_UNIBYTE_FALLBACK = 0x0001,
> +
> +  /* As an optimization, don't do multibyte processing
> +     when we know no multibyte characters are present.  */
> +  MBA_UNIBYTE_ONLY = 0x0002,
> +
> +  /* Don't add leading padding  */

Trailing period, for consistency with above?

> +  MBA_NO_LEFT_PAD = 0x0004,
> +
> +  /* Don't add trailing padding  */
> +  MBA_NO_RIGHT_PAD = 0x0008

Likewise?

>  #if 0 /* Other possible options.  */
>    /* Skip invalid multibyte chars rather than failing  */
> -  MBA_IGNORE_INVALID   = 0x0002,
> +  MBA_IGNORE_INVALID
>
>    /* Align multibyte strings using "figure space" (\u2007)  */
> -  MBA_USE_FIGURE_SPACE = 0x0004,
> -
> -  /* Don't add any padding  */
> -  MBA_TRUNCATE_ONLY    = 0x0008,
> +  MBA_USE_FIGURE_SPACE
>
>    /* Don't truncate  */
> -  MBA_PAD_ONLY         = 0x0010,
> +  MBA_NO_TRUNCATE
> +
> +  /* Ensure no leading whitepsace  */
> +  MBA_LSTRIP
> +
> +  /* Ensure no trailing whitepsace  */
> +  MBA_RSTRIP

More periods?

>  #endif
>  };
>
> diff --git a/gl/tests/test-mbsalign.c b/gl/tests/test-mbsalign.c
> index 86aa877..a65b1d9 100644
> --- a/gl/tests/test-mbsalign.c
> +++ b/gl/tests/test-mbsalign.c
> @@ -39,6 +39,32 @@ main (void)
>    n = mbsalign ("es", dest, sizeof dest, &width, MBS_ALIGN_CENTER, 0);
>    ASSERT (*dest == ' ' && *(dest + n - 1) == ' ');
>
> +  /* Test center alignment, with no trailing padding.  */
> +  width = 4;
> +  n = mbsalign ("es", dest, sizeof dest, &width, MBS_ALIGN_CENTER,
> +                MBA_NO_RIGHT_PAD);
> +  ASSERT (n == 3);
> +  ASSERT (*dest == ' ' && *(dest + n - 1) == 's');
> +
> +  /* Test left alignment, with no trailing padding. (truncate only).  */
> +  width = 4;
> +  n = mbsalign ("es", dest, sizeof dest, &width, MBS_ALIGN_LEFT,
> +                MBA_NO_RIGHT_PAD);

Is it worth inserting this?

    ASSERT (n == 2);

> +  ASSERT (*dest == 'e' && *(dest + n - 1) == 's');
> +
> +  /* Test center alignment, with no padding. (truncate only).  */
> +  width = 4;
> +  n = mbsalign ("es", dest, sizeof dest, &width, MBS_ALIGN_CENTER,
> +                MBA_NO_LEFT_PAD | MBA_NO_RIGHT_PAD);

Same question here.

> +  ASSERT (*dest == 'e' && *(dest + n - 1) == 's');
> +
> +  /* Test center alignment, with no left padding. (may be useful for RTL?)  
> */
> +  width = 4;
> +  n = mbsalign ("es", dest, sizeof dest, &width, MBS_ALIGN_CENTER,
> +                MBA_NO_LEFT_PAD);
> +  ASSERT (n == 3);
> +  ASSERT (*dest == 'e' && *(dest + n - 1) == ' ');
> +
>    if (setlocale (LC_ALL, "en_US.UTF8"))
>      {
>        /* Check invalid input is flagged.  */
> @@ -94,6 +120,12 @@ main (void)
>        n = mbsalign ("t\tés" /* 6 including NUL */ , dest, sizeof dest,
>                      &width, MBS_ALIGN_LEFT, 0);
>        ASSERT (n == 7);
> +
> +      /* Test forced unibyte truncation.  */
> +      width = 4;
> +      n = mbsalign ("t\tés", dest, sizeof dest, &width, MBS_ALIGN_LEFT,
> +                    MBA_UNIBYTE_ONLY);
> +      ASSERT (n == 4);
>      }
>
>    return 0;