This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [8/9] multiple locations
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