Multiexec MI
Vladimir Prus
vladimir@codesourcery.com
Wed Feb 24 07:53:00 GMT 2010
On Monday 22 February 2010 15:16:24 Pedro Alves wrote:
> On Friday 19 February 2010 20:15:40, Vladimir Prus wrote:
> > On Monday 08 February 2010 22:20:13 Pedro Alves wrote:
> >
> > > > - old_chain = make_cleanup_restore_current_thread ();
> > > > - iterate_over_threads (interrupt_thread_callback, &pid);
> > > > - do_cleanups (old_chain);
> > > > + struct inferior *inf = find_inferior_id (current_context->thread_group);
> > > > + iterate_over_threads (interrupt_thread_callback, &(inf->pid));
> > >
> > > Redundant ()'s.
> >
> > I think the version with () is more readable.
>
> Well, `without' is the defacto standard. See,
>
> with ()'s
>
> >egrep "&\(([a-zA-Z0-9_]+(\->|\.))+[a-zA-Z0-9_]*" * -rn | wc -l
> 45
>
> without:
>
> >egrep "&([a-zA-Z0-9_]+(\->|\.))+[a-zA-Z0-9_]*" * -rn | wc -l
> 1419
>
> Makes me wanna whack those 45 instances for consistency. ;-)
>
> >
> > > > if (parse->thread != -1)
> > > > {
> > > > struct thread_info *tp = find_thread_id (parse->thread);
> > > > @@ -1767,6 +1908,8 @@ mi_cmd_execute (struct mi_parse *parse)
> > > > error (_("Invalid frame id: %d"), frame);
> > > > }
> > > >
> > > > + current_context = parse;
> > > > +
> > >
> > > Hmm, aren't the `struct mi_parse' objects leaking
> > > for every MI command? I can't see where they're released in
> > > mi_execute_command ?
> >
> > Close to the end of that function, I see:
> >
> > mi_parse_free (command);
> >
> > Is it not there for you?
>
> Ah, there it is. Thanks.
>
> ( it leaks if bpstat_do_actions throws, though ;-) )
>
> >
> > > In the latter strncmp above:
> > >
> > > + if (strncmp (chp, "--all", as) == 0)
> > >
> > > AS is always larger than strlen("--all"), so the
> > > strncmp's check on AS is useless and confusing here.
> > > I think you wanted:
> > >
> > > if (strcmp (chp, "--all") == 0)
> > > {
> > > parse->all = 1;
> > > chp += strlen (chp);
> > > }
> >
> > I've adjusted sizes. I prefer to keep two ifs with the same structure.
>
> But now it accepts --allfoofoofoo, and isn't checking that --all
> is the last token in the input as is described, when my suggestion
> did not have such issues. The current code reads:
>
> /* See if this --all as the last token in the input.
> Both the string and count are smaller by 1. */
> if (strncmp (chp, "--all", as - 1) == 0)
> {
> parse->all = 1;
> chp += (as - 1);
> }
>
> (also, typo: s/--all as the last/--all is the last/)
>
>
> I also spotted:
>
> > - -exec-run
> > + -exec-run [--all | --thread-group N ]
> ^ -- inconsistent -- ^
>
> > +@deftypefun void inferior_removed (struct inferior *@var{inf})
> > +The inferior @var{inf} has been removed from the list of inferiors.
> > +This method is called immediate before freeing @var{inf}.
> ^^^^^^^^^
> s/immediate/immediately
>
>
> Otherwise, still looks okay to me. Thanks.
Thanks. Here's the version I have checked in.
Thanks,
--
Vladimir Prus
CodeSourcery
vladimir@codesourcery.com
(650) 331-3385 x722
-------------- next part --------------
A non-text attachment was scrubbed...
Name: multiprocess-mi-cvs-final.diff
Type: text/x-patch
Size: 41247 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20100224/1aa27a8a/attachment.bin>
More information about the Gdb-patches
mailing list