[PATCH PR/42686] Align the help text output

[Shujing Zhao]

On 03/22/2010 04:41 PM, Shujing Zhao wrote:
> 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
> 
Added the ChangeLog.
Is it OK?
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ChangeLog
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20100331/78f47c11/attachment.ksh>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0331.patch
Type: text/x-patch
Size: 8370 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20100331/78f47c11/attachment.bin>