[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: maint: update the mbsalign module
From: |
Jim Meyering |
Subject: |
Re: maint: update the mbsalign module |
Date: |
Tue, 16 Mar 2010 08:28:24 +0100 |
Pádraig Brady wrote:
> I'm updating this module with a view to fixing
> alignment issues in df etc. soon.
Great!
That has been a sore point for a long time.
> This fixes an unlikely over truncation bug
> and cleans up the interface so that one doesn't
> have to check for errors in all cases.
>
> I've also synced this with the util-linux-ng project
> for alignment in cal and fdisk et. al. there.
...
> Subject: [PATCH] maint: update the mbsalign module
>
> * gl/lib/mbsalign.c (mbsalign): Support the MBA_UNIBYTE_FALLBACK
> flag which reverts to unibyte mode if one can't allocate memory
> or if there are invalid multibyte characters present.
> Note memory is no longer dynamically allocated in unibyte mode so
> one can assume that mbsalign() will not return an error if this
> flag is present. Fix an error where we would truncate too many
> characters in the presence of single byte non printable chars.
> Suppress a signed/unsigned comparison warning.
> (ambsalign): A new wrapper function to dynamically allocate
> the minimum memory required to hold the aligned string.
> * gl/lib/mbsalign.h: Add the MBA_UNIBYTE_FALLBACK flag and
> also document others that may be implemented in future.
> (ambsalign): A prototype for the new wrapper.
These look like fine improvements.
Have you considered dividing it into two or more change sets?
Mixing "fix" and "clean-up" changes in one change-set is
best avoided, if only to ease review and bisection.
In addition, it would ease long-term maintenance if you would
at least outline how to exercise the bug(s?) you're fixing.
Then I'll try to make time to write a test for it.
> ---
> gl/lib/mbsalign.c | 101 +++++++++++++++++++++++++++++++++++++++++-----------
> gl/lib/mbsalign.h | 23 ++++++++++++
> 2 files changed, 102 insertions(+), 22 deletions(-)
>
> diff --git a/gl/lib/mbsalign.c b/gl/lib/mbsalign.c
...
> size_t
> mbsalign (const char *src, char *dest, size_t dest_size,
...
> - if (conversion || (n_cols > *width))
> + if (wc_enabled && (conversion || (n_cols > *width)))
> {
> - newstr = malloc (src_size);
> - if (newstr == NULL)
> - goto mbsalign_cleanup;
> - str_to_print = newstr;
> - if (wc_enabled)
> - {
> - n_cols = wc_truncate (str_wc, *width);
> - n_used_bytes = wcstombs (newstr, str_wc, src_size);
> - }
> - else
> + if (conversion)
> + {
> + /* May have increased the size by converting
> + \t to \uFFFD for example. */
> + src_size = wcstombs(NULL, str_wc, 0) + 1;
missing space-before open-paren
...
> +/* A wrapper around mbsalign() to dynamically allocate the
> + minimum amount of memory to store the result.
> + NULL is returned on failure. */
Use the active voice:
"Return NULL upon failure."
> +
> +char *
> +ambsalign (const char *src, size_t *width, mbs_align_t align, int flags)
> +{
...