Re: misalignment in ls -l in fr_FR locale

[Jim Meyering]

Pádraig Brady wrote:
> I expect to push the attached updated patch soon.

Hi Pádraig,

Thanks for working on this, but please hold off until after 7.2.
I'm trying to stabilize for a bug-fix(was "-only") release.

> Subject: [PATCH] ls: Fix alignment when month names have varying widths
...
> diff --git a/gl/lib/mbsalign.c b/gl/lib/mbsalign.c
...

I much prefer to order functions (defined before used) so that
static prototypes are not needed.  Then, when you change the return
type of e.g., wc_ensure_printable to "bool" you'll have to change it
in only one place.

> +static int wc_ensure_printable (wchar_t * wchars);
> +static size_t wc_truncate (wchar_t * wchars, size_t width);
> +static int rpl_wcswidth (const wchar_t *s, size_t n);
> +
> +/* Align a string in a given screen width, handling multi-byte characters.
> +   In addition if the string is too large for the width it's truncated.

use active voice, i.e.,

   In addition if the string is too large for the width, truncate it to fit.

> +   When centering, the number of trailing spaces may be 1 less than the
> +   number of leading spaces.
> +   Returned is the number of bytes written to or required in dest (without

Return the number...

> +   the trailing NUL).  A value >= dest_size means there wasn't enough space.
> +   The width parameter both specifies the width to align/pad/truncate to,
> +   and is updated to return the width used before padding.  */

Would "desired_width" be a better parameter name for dest_size?
"size" makes me think of a buffer size, i.e., number of bytes allocated.

> +int
> +mbsalign (const char *src, char *dest, size_t dest_size,
> +          int *width, mbs_align_t align)
> +{
> +  int src_len = strlen (src);

Please use size_t, not int for anything length-related.
And especially for things you pass to malloc.

> +  int ret = 0;
> +  char *newstr = NULL;
> +  wchar_t *str_wc = NULL;
> +  const char *str_to_print = src;
> +  int used = src_len, spaces, wc_conversion = 0, wc_enabled = 0;

The name "used" could mean something boolean, or a length.
Call it "n_used" and there is no ambiguity.
n_spaces would be clearer, in the same way.
use size_t for those two.

wc_conversion and wc_enabled should be of type "bool".

> +  if (MB_CUR_MAX > 1)
> +    {
> +      int src_chars = mbstowcs (NULL, src, 0) + 1;
> +      str_wc = xmalloc (src_chars * sizeof (wchar_t));

This function would be more generally useful (usable in a library) if it
did not rely on xmalloc (i.e., use malloc instead and handle failure),
and probably not much harder to implement.  maybe worth the trouble...

> +      if (mbstowcs (str_wc, src, src_chars) > 0)
> +        {
> +          str_wc[src_chars - 1] = L'\0';
> +          wc_enabled = 1;
> +          wc_conversion = wc_ensure_printable (str_wc);
> +          used = rpl_wcswidth (str_wc, src_chars);
> +        }
> +    }
> +
> +  if (wc_conversion || used > *width)
> +    {
> +      newstr = xmalloc (src_len);
> +      str_to_print = newstr;
> +      if (wc_enabled)
> +        {
> +          used = wc_truncate (str_wc, *width);
> +          wcstombs (newstr, str_wc, src_len);
> +        }
> +      else
> +        {
> +          memcpy (newstr, src, *width);
> +          newstr[*width] = '\0';
> +        }
> +    }
> +
> +  spaces = *width - used;
> +  spaces = (spaces < 0 ? 0 : spaces);
> +  *width = used;  /* indicate to called how many cells used.  */
> +
> +  /* FIXME: Should I be padding with "figure space" (\u2007)
> +     rather than spaces below? (only if non ascii data present)  */
> +  switch (align)
> +    {
> +    case MBS_ALIGN_CENTER:
> +      ret = snprintf (dest, dest_size, "%*s%s%*s",
> +                      spaces / 2 + spaces % 2, "",
> +                      str_to_print, spaces / 2, "");

For a potential-library function like this, I'd be inclined
to use stpcpy+memchr rather than the heavy-weight snprintf.

> +      break;
> +    case MBS_ALIGN_LEFT:
> +      ret = snprintf (dest, dest_size, "%s%*s", str_to_print, spaces, "");
> +      break;
> +    case MBS_ALIGN_RIGHT:
> +      ret = snprintf (dest, dest_size, "%*s%s", spaces, "", str_to_print);
> +      break;
> +    }
> +
> +  free (str_wc);
> +  free (newstr);
> +
> +  return ret;
> +}
> +
> +/* Replace non printable chars.
> +   Return 1 if replacement made, 0 otherwise.  */
> +
> +static int

static bool

> +wc_ensure_printable (wchar_t * wchars)

this spacing, "wchar_t * wchars", looks like an artifact of running
the code through indent.  You can make it format properly by
adding -Twchar_t to ~/.indent.pro.  I wonder if GNU indent is
still maintained...

> +{
> +  int replaced = 0;

bool

> +  wchar_t *wc = wchars;
> +  while (*wc)
> +    {
> +      if (!iswprint ((wint_t) * wc))

spacing: s/ wc/wc/

> +        {
> +          *wc = 0xFFFD; /* L'\uFFFD' (replacement char) */
> +          replaced = 1;

... = true

> +        }
> +      wc++;
> +    }
> +  return replaced;
> +}

I'm stopping here, for now.