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

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

On Fri, 19 Mar 2010, Shujing Zhao wrote:

> > 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.
> It is changed to *length--* at the new logic.
> +      colwidth += wcwidth (wc[0]);
> +      length--;
> +      if (length == 0)
> +       len = remaining;
> I have tried to used length = length - nbytes to get the new length. But if
> nbytes > 1 and the character is the last character of the string (length will
> be 0 if it decreases nbytes), it would not assign the last variable *len* to
> *len_bk*, then the character that maybe put into the ROOM would not be
> printed. That is to say, it presumes the nbytes is 1. If (nbytes > 1), it
> would be handled specially (just to update the value of len_bk). At the other
> conditionals, len_bk is always kept consistently with len.

I'm afraid none of this makes any sense to me, because I have no idea what 
"length", "len" and "len_bk" are meant to be, and how they differ.  You 
need clearer variable names.  I think you also need comments at various 
points stating what the values various variables mean at those points, and 
maybe an overview comment about the algorithm used.  It would be best to 
avoid ambiguous terms such as "length" at all points in variable names and 
comments, instead talking about numbers of bytes difference between two 
points, or similar.

For example, the present algorithm in opts.c:wrap_help might be described 
thus:

/* Look for a suitable point at which to break HELP.  If there is one 
   after at most ROOM columns, take the latest such point.  Otherwise, 
   take the first such point in the string, or the end of the string if it 
   cannot be broken.  */

If the new algorithm is still like that, you may just need to update 
variable names (and explain what the interface is for indicating both how 
much of the string to print, and how much (spaces) to skip before starting 
on the next line).

The key variables to explain are LEN and I.  At each iteration in the 
present loop, something like this might explain them.

/* The best candidate point found so far to break HELP is after LEN 
   bytes, where either LEN < I, or LEN == REMAINING if the string cannot 
   be broken within the first I bytes.  If we are outside ROOM columns and 
   have found somewhere to break the string, that is the best point 
   available; otherwise, breaking after I bytes will be better if that is 
   a valid point to break.  */

Now, things will be a bit more complicated in that you need to track the 
current column width as well as the current number of bytes of the string 
that take up that width.  But I think the basic idea is still the same - 
it is just that "length", "len" and "len_bk" are not useful variable names 
to distinguish the different quantities being tracked and so make clear 
whether particular arithmetic you are doing on them makes logical sense.  
I remain very suspicious of anything modifying a byte counter by 1 in this 
context; if you are ever modifying a byte counter by something other than 
the number of bytes in the relevant multibyte character I think you are 
confused and need to sort out the logic of the code so you don't need to 
do so.

-- 
Joseph S. Myers
joseph@codesourcery.com