From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5783 invoked by alias); 22 Feb 2011 08:51:30 -0000 Received: (qmail 5774 invoked by uid 22791); 22 Feb 2011 08:51:30 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD 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; Tue, 22 Feb 2011 08:51:24 +0000 Received: (qmail 24970 invoked from network); 22 Feb 2011 08:51:23 -0000 Received: from unknown (HELO scottsdale.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 22 Feb 2011 08:51:23 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [commit] fix for "info threads" printing multiple headers Date: Tue, 22 Feb 2011 09:17:00 -0000 User-Agent: KMail/1.13.5 (Linux/2.6.35-25-generic; KDE/4.6.0; x86_64; ; ) Cc: Michael Snyder References: <4D62F7D6.8040705@vmware.com> In-Reply-To: <4D62F7D6.8040705@vmware.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201102220851.20252.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: 2011-02/txt/msg00574.txt.bz2 Thanks! A few comments below. If you don't want to fix these, I'll try to find a bit later on myself. On Monday 21 February 2011 23:40:06, Michael Snyder wrote: > +int > +number_is_in_list (char *list, int number) > +{ > + if (list == NULL || *list == '\0') > + return 1; > + > + while (list != NULL && *list != '\0') > + if (get_number_or_range (&list) == number) > + return 1; > + > + return 0; > +} - why the 'list != NULL' check in the while loop? get_number_or_range never puts NULL in the output argument, and NULL lists are short circuited above. - get_number_or_range mantains an internal state machine using static variables. I think that as long as you always pass in the same list string, and the list spec string is correctly formed, you're not hitting stale state inside get_number_or_range. It'd be nicer if get_number_or_range (or a variant which get_number_or_range would then be implemented on top of) took an additional struct pointer that pointed to a struct that held all the currently static state. This function would then always pass in a pointer into such a newly initialized object on the stack. - get_number_or_range returns '0' on error. You can see a bunch of its old callers in breakpoint.c handling that '0' case specially. It comes from get_number_trailer: /* Return zero, which caller must interpret as error. */ retval = 0; I suspect that we'll want to make than an "error" call or to adjust the interface of these functions to allow returning an error indication unambiguous with zero the number, as soon as we want to reuse this function in places where zero makes sense as valid number... Examples of not handling the special zero: (gdb) info threads a 1 Id Target Id Frame * 1 Thread 0x7ffff7fd6700 (LWP 13769) "threads" 0x00007ffff7bc7285 in pthread_join () from /lib/libpthread.so.0 (gdb) (gdb) info threads a-1 No threads match 'a-1'. (gdb) info threads a -1 Id Target Id Frame * 1 Thread 0x7ffff7fd6700 (LWP 13769) "threads" 0x00007ffff7bc7285 in pthread_join () from /lib/libpthread.so.0 (gdb) The last one only "works" by accident due to the extra space. -- Pedro Alves