From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16889 invoked by alias); 2 Feb 2010 23:57:22 -0000 Received: (qmail 16880 invoked by uid 22791); 2 Feb 2010 23:57:21 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 02 Feb 2010 23:57:13 +0000 Received: from int-mx03.intmail.prod.int.phx2.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.16]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o12NvCsT006234 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 2 Feb 2010 18:57:12 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx03.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o12NvAn6026516; Tue, 2 Feb 2010 18:57:10 -0500 Received: from opsy.redhat.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id o12Nv9jh014031; Tue, 2 Feb 2010 18:57:09 -0500 Received: by opsy.redhat.com (Postfix, from userid 500) id 2434837998B; Tue, 2 Feb 2010 16:57:09 -0700 (MST) From: Tom Tromey To: Chris Moller Cc: gdb-patches@sourceware.org Subject: Re: Patch for PR 10728 References: <4B689FD2.9070300@redhat.com> Reply-To: tromey@redhat.com Date: Tue, 02 Feb 2010 23:57:00 -0000 In-Reply-To: <4B689FD2.9070300@redhat.com> (Chris Moller's message of "Tue, 02 Feb 2010 16:57:38 -0500") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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/msg00051.txt.bz2 Chris> + * c-lang.h: Empty entry; it is customary to list the new names, e.g., from an earlier patch: * c-lang.h (cp_print_value_fields_rtti): Declare. Chris> +extern void cp_initialise_statmem_stack(); Chris> +extern void cp_free_statmem_stack(); Space before open parens. Use (void), not (). These occur several times. Chris> - return val_print (val_type, value_contents_all (val), Chris> + { Chris> + int rc; Chris> + Chris> + cp_initialise_statmem_stack(); initialize, with a z. Chris> + rc = val_print (val_type, value_contents_all (val), Chris> value_embedded_offset (val), Chris> value_address (val), Chris> stream, 0, &opts, current_language); Chris> + Chris> + cp_free_statmem_stack(); There are other calls to val_print in this function. It seems like they should all be wrapped. If cp_free_statmem_stack must be called, then you need a cleanup. However, does it really need to be called? IMO it is ok to keep a little data on this obstack in between calls as long as it is cleared at the next call (so that we don't leak an unbounded amount over time). It isn't clear to me from this patch or the explanation that cp_initialise_statmem_stack is called at every relevant entry point. In fact it would be better not to have to do that, because auditing this call hierarchy is tricky. E.g., m2-valprint.c has a call to cp_print_value_fields. Does that matter? I didn't read the old code in depth; is there a way to make this self-initializing? Chris> +/* initialise to empty the static member stack */ Comments should start with a capital and end with a period & two spaces. See the GNU Coding Standards. Chris> + void * statmem_obstack_top = NULL; No space after the "*". Chris> - first_dont_print Chris> - = (CORE_ADDR *) obstack_base (&dont_print_statmem_obstack); Chris> - i = (CORE_ADDR *) obstack_next_free (&dont_print_statmem_obstack) Chris> - - first_dont_print; Chris> + first_dont_print = Chris> + (CORE_ADDR *)obstack_base (&dont_print_statmem_obstack); Chris> + i = Chris> + obstack_object_size (&dont_print_statmem_obstack) / sizeof(CORE_ADDR); The formatting is weird here; the first statement doesn't appear to have changed, but has gratuitous formatting changes, and the second statement has a tab before the "=". Chris> +send_gdb "print b\n" Chris> + Chris> +gdb_expect { Chris> + -re "same as static member" { Chris> + pass "print b" Chris> + } Chris> + timeout { fail "(timeout) print b" } Chris> +} Don't use sent + gdb_expect. Use gdb_test. Tom