Re: Multiexec MI

[Pedro Alves <pedro at codesourcery dot com>]

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.

-- 
Pedro Alves