Re: [8/9] multiple locations

[Vladimir Prus <vladimir@codesourcery.com>]

On Saturday 08 September 2007 15:46:48 Eli Zaretskii wrote:
> > From: Vladimir Prus <vladimir@codesourcery.com>
> > Date: Sat, 8 Sep 2007 02:48:52 +0400
> > 
> > @@ -3397,30 +3448,79 @@ print_one_breakpoint (struct breakpoint 
> >    struct cleanup *old_chain = make_cleanup_ui_out_stream_delete (stb);
> >    struct cleanup *bkpt_chain;
> >  [...]
> > -  ui_out_field_int (uiout, "number", b->number);
> > +  if (part_of_multiple)
> > +    {
> > +      char buf[30];
> 
> Why 30?  GNU coding standards frown on arbitrary constant sizes.
> 
> > +      sprintf (buf, "%d.%d", b->number, loc_number);

Clearly, '30' is sufficient for 2^32 breakpoints, each having 2^32 locations.
I don't think using variable-size buffer is going to have any difference.

> > +      ui_out_field_string (uiout, "number", buf);
> 
> I'm worried about this N.M thing: when the number of locations takes
> 2 or 3 digits, won't it disrupt the display alignment?

This is valid concern. Now the column width for number is 3, which is easy to
overflow. I think increasing that is in order.

> 
> > -  ui_out_field_fmt (uiout, "enabled", "%c", bpenables[(int) b->enable_state]);
> > -  ui_out_spaces (uiout, 2);
> > +  if (part_of_multiple)
> > +    ui_out_field_string (uiout, "enabled", 
> > +			 loc->shlib_disabled 
> > +			 ? (loc->enabled ? "y(p)" : "n(p)")
> > +			 : (loc->enabled ? "y" : "n"));
> 
> These changes in the breakpoint display should be documented in the
> user manual.

As usual, I'll only send doc changes when the code is reviewed. My overview
email talks about those changes, so should be sufficient for review.

> 
> > +/* set_raw_breakpoint() is a low level routine for allocating and
> 
> Another GNU coding standards thing: foo() should not be used as an
> indication that `foo' is a function.  Just say `foo' instead; foo()
> looks like a call to `foo' with no arguments, which is not what you
> mean.

Well, in fact this comment existed before my changes.

> > @@ -7579,6 +7874,13 @@ disable_command (char *args, int from_tt
> >        default:
> >  	continue;
> >        }
> > +  else if (strchr (args, '.'))
> > +    {
> > +      struct bp_location *loc = find_location_by_number (args);
> > +      if (loc)
> > +	loc->enabled = 0;
> > +      check_duplicates (loc->owner);
> > +    }
> 
> This new feature needs documentation.

Yes, see above.

- Volodya