Re: [RFA] info break/watch/trace use get_number_or_range, take two

[Michael Snyder <msnyder@vmware.com>]

Pedro Alves wrote:
> On Monday 21 February 2011 22:23:28, Michael Snyder wrote:
>> Supercedes previous submission.
>>
>> Fixed Pedro's issues.  Tested with maint info break.
> 
> "Pedro's issues"?  Eh, I have no personal issues!
> 
> Thanks, I like this much better.
> 
>> +      /* If we have an "args" string, it is a list of breakpoints to 
>> +        accept.  Skip the others.  */
>> +      if (args != NULL && *args != '\0')
>> +       {
>> +         if (allflag && (parse_and_eval_long (args) != b->number))
> 
> Spurious parens?

OK

>> +           continue;
>> +         if (!allflag && !number_is_in_list (args, b->number))
>> +           continue;
>> +       }
> 
> I think that parse_and_eval_long means that
> 
>   "maint info break 2-3" will print breakpoint -1,
> 
> while 
> 
>   "info break 2-3" will print breakpoints 2 and 3
> 
> 
> It's fine with me, just pointing it out.

Just preserving the existing behavior.

>> -           addr_bit = breakpoint_address_bits (b);
>> -           if (addr_bit > print_address_bits)
>> -             print_address_bits = addr_bit;
>> +      if (allflag || user_breakpoint_p (b))
>> +       {
>> +         int addr_bit, type_len;
>>  
>> -           type_len = strlen (bptype_string (b->type));
>> -           if (type_len > print_type_col_width)
>> -             print_type_col_width = type_len;
>> +         addr_bit = breakpoint_address_bits (b);
>> +         if (addr_bit > print_address_bits)
>> +           print_address_bits = addr_bit;
>>  
>> -           nr_printable_breakpoints++;
>> -         }
>> -      }
>> +         type_len = strlen (bptype_string (b->type));
>> +         if (type_len > print_type_col_width)
>> +           print_type_col_width = type_len;
>> +
>> +         nr_printable_breakpoints++;
>> +       }
>> +    }
>>  
>>    if (opts.addressprint)
>>      bkpttbl_chain 
>> @@ -5169,16 +5178,16 @@ breakpoint_1 (int bnum, int allflag, 
>>      annotate_field (3);
>>    ui_out_table_header (uiout, 3, ui_left, "enabled", "Enb");   /* 4 */
>>    if (opts.addressprint)
>> -       {
>> -         if (nr_printable_breakpoints > 0)
>> -           annotate_field (4);
>> -         if (print_address_bits <= 32)
>> -           ui_out_table_header (uiout, 10, ui_left, 
>> -                                "addr", "Address");            /* 5 */
>> -         else
>> -           ui_out_table_header (uiout, 18, ui_left, 
>> -                                "addr", "Address");            /* 5 */
>> -       }
>> +    {
>> +      if (nr_printable_breakpoints > 0)
>> +       annotate_field (4);
>> +      if (print_address_bits <= 32)
>> +       ui_out_table_header (uiout, 10, ui_left, 
>> +                            "addr", "Address");                /* 5 */
>> +      else
>> +       ui_out_table_header (uiout, 18, ui_left, 
>> +                            "addr", "Address");                /* 5 */
>> +    }
> 
> Please let's keep big formatting changes separate from
> functional changes.  We ask that in patch submissions, so we
> should follow the same practice ourselves.  It's really a good
> rule BTW.  I think of that as one case of what I call it
> The Principle of Least Reversibility (TM), so explained:
> If by chance this patch needs to be reverted, you don't want
> to revert the whitespace fixes.  Hence, you'll want to make
> the whitespace fixes a separate patch.  The same rule applies
> to preparatory fixes and changes, and code moves (where if
> you move the code, you should NOT change it in any other way).
> 
>>    add_info ("breakpoints", breakpoints_info, _("\
>> -Status of user-settable breakpoints, or breakpoint number NUMBER.\n\
>> +Status of user-settable breakpoints listed, or all breakpoints if no argument.\n\
> 
> "listed" doesn't sound obviously referring to the spec
> you pass as argument to the command.  "listed where?  the
> command itself is printing a list." was my thought.  Is
> there any other way to spell that?

It's getting hard to describe.  Do you have a suggestion?