Re: [PATCH PR/42686] Align the help text output

["Joseph S. Myers" <joseph@codesourcery.com>]

On Thu, 18 Mar 2010, Shujing Zhao wrote:

>  
> +static wchar_t *
> +get_sub_wstr (wchar_t *wstr, int len)

Needs comment explaining semantics of parameters and return value.

> +/* Return the number of bytes in the multibyte representation of the wide 
> +   character WC. */
> +
> +static int
> +wctomb_nbytes (wchar_t wc)
> +{

This code is heavily assuming that converting from multibyte to wide 
characters and back again yields exactly the same multibyte characters.  
This is more likely in the Unicode world than it used to be, but it's not 
clear we should be presuming it.  I think code without this presumption 
would be simpler than code with it; rather than converting the whole 
string at once and then repeatedly extracting substrings, you'd convert 
one character at a time, keeping track of the width at each point 
(computing the width of one character at a time) and of the byte count to 
the last possible point for a line break.  (That way, you could also 
easily allow for encodings with shift sequences, keeping an mbstate_t in 
the course of processing the string.)

> +/* Return the length of multibyte string MSGSTR that can be printed within the

"length" = number of bytes?

> +    { 
> +      nbytes = wctomb_nbytes (wmsgstr [i]);
> +      colwidth ++;

Why this increment?  Characters may have width 0 (combining characters) or 
more than 1 and it's not clear why adding 1 should be the correct thing to 
do here.

> +      else if (nbytes > 1)

Why this conditional?  Whatever the number of bytes is, you need to update 
the width appropriately.

> +          {
> +	    if (colwidth > room)	
> +              len = len_bk;
> +            break;

Indentation seems very confused.  Make sure that new code consistently 
uses TABs for indenting source code lines.

> +      int n_spaces = item_width <= item_col_width 
> +                     ? col_width - item_width
> +                     : col_width - item_col_width;

I don't see why this conditional is here.  For printing spaces, it's 
always the number of columns that's relevant; the number of bytes is 
irrelevant.

-- 
Joseph S. Myers
joseph@codesourcery.com