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