From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8358 invoked by alias); 14 Nov 2008 01:53:02 -0000 Received: (qmail 8041 invoked by uid 22791); 14 Nov 2008 01:52:58 -0000 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 14 Nov 2008 01:52:22 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id F21101EEB6F; Thu, 13 Nov 2008 20:52:19 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id yoBjJzZaUK15; Thu, 13 Nov 2008 20:52:19 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id A5EC21EEB5E; Thu, 13 Nov 2008 20:52:19 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 8881FE7ACD; Thu, 13 Nov 2008 17:52:17 -0800 (PST) Date: Fri, 14 Nov 2008 11:46:00 -0000 From: Joel Brobecker To: Vladimir Prus Cc: gdb-patches@sources.redhat.com Subject: Re: [RFC] Implement -list-thread-groups. Message-ID: <20081114015217.GD12802@adacore.com> References: <200811122333.29218.vladimir@codesourcery.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200811122333.29218.vladimir@codesourcery.com> User-Agent: Mutt/1.4.2.2i 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: 2008-11/txt/msg00318.txt.bz2 > I'll commit in a few days if there are no objections. If others are fine with this too, then so am I. But I don't think this is the normal procedure: For non-MI changes, my understanding is that you're supposed to wait for approval unless the changes are obvious (particularly for patches labeled RFC). I don't blame you for doing this, given the poor track record we're having in terms of speed of review - I guess we need more help. Anyway, onto the patch... > * thread.c (print_thread_info): New parameter pid, to print > threads of specific process. > * gdbthread.h (print_thread_info): New parameter pid. > * mi/mi-cmds.c (mi_cmds): Register -list-thread-groups. > * mi/mi-cmds.h (mi_cmd_list_thread_groups): New. > * mi/mi-main.c (mi_cmd_thread_info): Adjust. > (print_one_process, mi_cmd_list_thread_groups): New. Overall, looks good to me. Just a couple of minor nits and a question. > void > -print_thread_info (struct ui_out *uiout, int requested_thread) > +print_thread_info (struct ui_out *uiout, int requested_thread, int pid) Can you add a comment for the new parameter in the function documentation, please? > - > + You're adding some spaces in what would otherwise be an empty line. Can you get rid of that change? > + if (pid == -1 && requested_thread == -1 ) > { > gdb_assert (current_thread != -1 > || !thread_list); This has little to do with your change, but I'm curious. What does this code do? It seems to be adding a current-thread-id field in the output, but why only doing it when requested_thread (and pid) is -1? -- Joel