From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10811 invoked by alias); 22 Feb 2010 12:16:33 -0000 Received: (qmail 10797 invoked by uid 22791); 22 Feb 2010 12:16:32 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 22 Feb 2010 12:16:28 +0000 Received: (qmail 25024 invoked from network); 22 Feb 2010 12:16:27 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 22 Feb 2010 12:16:27 -0000 From: Pedro Alves To: Vladimir Prus Subject: Re: Multiexec MI Date: Mon, 22 Feb 2010 12:16:00 -0000 User-Agent: KMail/1.12.2 (Linux/2.6.31-19-generic; KDE/4.3.2; x86_64; ; ) Cc: gdb-patches@sourceware.org References: <201001132329.30212.vladimir@codesourcery.com> <201002081920.13920.pedro@codesourcery.com> <201002192315.40134.vladimir@codesourcery.com> In-Reply-To: <201002192315.40134.vladimir@codesourcery.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201002221216.24109.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-02/txt/msg00523.txt.bz2 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