On 03/19/2010 09:26 PM, Joseph S. Myers wrote:
> 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.  
Thanks. I changed the "len_bk" to "pre_len_to_print" as the number of bytes that 
have been found can be printed at ROOM columns, "len" to "cur_len_to_print" as 
the possible number of bytes that can be printed at ROOM columns, "length" to 
left_len as the number of bytes that MSGSTR left to convert to wide character.
Some comments are also added as your suggestion.

> 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.
Thanks. You are right. I found the way to solve the problem that I have 
mentioned when I explain why I have to use "length--". It just needs to store 
the previous point that can be break the string before the new point is set to 
the end of the string.

+      left_len -= nbytes;
+
+      /* Compute the point to break the string. */
+      if (left_len == 0)
+       {
+         pre_len_to_print = cur_len_to_print;
+         cur_len_to_print = remaining;
+       }

Is it Ok?

Thanks
Pearly