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

[Pedro Alves <pedro@codesourcery.com>]

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?

> +           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.


> -           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?

-- 
Pedro Alves