From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19100 invoked by alias); 11 Feb 2010 09:30:08 -0000 Received: (qmail 19085 invoked by uid 22791); 11 Feb 2010 09:30:06 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 11 Feb 2010 09:30:01 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id F2EFC2BABFA; Thu, 11 Feb 2010 04:29:58 -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 hAYYYE2+2htG; Thu, 11 Feb 2010 04:29:58 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 63A5C2BABF8; Thu, 11 Feb 2010 04:29:58 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id B146EF59AE; Thu, 11 Feb 2010 13:29:50 +0400 (RET) Date: Thu, 11 Feb 2010 09:30:00 -0000 From: Joel Brobecker To: Chris Moller Cc: gdb-patches@sourceware.org Subject: Re: pr 11067 patch Message-ID: <20100211092950.GC2907@adacore.com> References: <4B737180.4050802@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4B737180.4050802@redhat.com> User-Agent: Mutt/1.5.20 (2009-06-14) 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/msg00281.txt.bz2 > Provides a little more info on enums for simple 'p ' cases; > keeps the old format for complex cases like structs and arrays: I feel really bad about this, and I really apologize - I am just only suddenly wondering why this is considered a good idea, was that discussed? Please understand that this is not an objection, but I just had a look at the PR, and I happen to disagree with the reporter. According to me, he said: 1. If I print 'e', GDB prints 'Val1' and that's OK. 2. If I print 'Val1', GDB prints also prints 'Val1' and he says that, instead, GDB should print its numerical value. I disagree on (2) because, if he wanted the numerical value, he should have told GDB. For instance: (gdb) p GREEN $1 = GREEN (gdb) p /d GREEN $2 = 1 If people still think that this suggestion is a good one, I looked at the patch (the least I could do)... > +Wed Feb 10 17:13:44 2010 Chris Moller > + > + PR gdb/11067 > + * c-valprint.c (c_val_print): In case TYPE_CODE_ENUM, add code to > + print the numeric value of the enum and the enum tag for > + top-level, non-summary "print enum"s. > + if (options->summary || recurse != 0) > + { > + fputs_filtered (TYPE_FIELD_NAME (type, i), stream); > + } We do not use the curly braces in this case, when the block only contains one statement. > + else > + { > + char *enum_name; > + > + if (TYPE_NAME (type)) enum_name = TYPE_NAME (type); > + else if (TYPE_TAG_NAME(type)) enum_name = TYPE_TAG_NAME(type); > + else enum_name = ""; Can you place each statement on their own line? I am guessing that GNU indent will fix that anyway, and I also think that this is harder to read. Rather than "unknown", I wonder if we shouldn't be using "". > + fprintf_filtered (stream, "%s = (enum %s)%lld", > + TYPE_FIELD_NAME (type, i), > + enum_name, > + val); Just a gotcha, here: val is a LONGEST, which is not necessarily a long long. Use plongest to format your value into a string. Also, would you mind adding a short comment explaining why you are doing all this (we want to be concise if printing this enum as part of a larger data structure or while in summary mode). > + * gdb.cp/classes.exp (multiple places): > + * gdb.cp/m-data.exp (multiple places): > + * gdb.cp/m-static.exp (multiple places): > + * gdb.cp/namespace.exp (multiple places): > + * gdb.mi/mi-var-display.exp (multiple places): > + * gdb.mi/mi2-var-display.exp (multiple places): > + * gdb.python/py-value.exp (multiple places): > + * gdb.base/setvar.exp (multiple places): Updated expects to new > + enum format. Hmmm, I don't think you need the "multiple places". IIRC, the GCS allow you to just say: * gdb.base/setvar.exp: Update throughout to match new enum format. > +enum E e = Val1; > + > +enum E ea[] = { Val1, Val2, Val1 }; > + > +struct Es es = { 5, Val2 }; > + > +int main() { > + return 0; > +} I can see some linkers optimizing away your global variables, causing the testcase to fail... The AIX linker, for instance, does that. You have a look at exprs.c, or grep for AIX in gdb.base/*.c for some ideas on how to deceive the linker and prevent this unwanted optimization. > +if { [skip_cplus_tests] } { continue } > + > +load_lib "cp-support.exp" I don't think this is right, is it? Since this is a C test, I don't understand why that would be needed... > +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {c++ debug}] != "" } { > + untested pr11067.exp > + return -1 Same here, you're doing a C++ build for a C program. Have a look at http://sourceware.org/gdb/wiki/GDBTestcaseCookbook. If I were you, I'd just do the following to build your program: set testfile template set srcfile ${testfile}.c if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } { return -1 } The above also includes the calls to gdb_exit/gdb_start/etc, so they can also go. > +# set a breakpoint at the return stmt Superfluous comment :). > +gdb_exit > +return 0 Also superfluous... -- Joel