From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11397 invoked by alias); 26 Sep 2008 05:03:05 -0000 Received: (qmail 11387 invoked by uid 22791); 26 Sep 2008 05:03:04 -0000 X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (66.187.233.31) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 26 Sep 2008 05:02:00 +0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id m8Q4j6Th032434; Fri, 26 Sep 2008 00:45:06 -0400 Received: from pobox.stuttgart.redhat.com (pobox.stuttgart.redhat.com [172.16.2.10]) by int-mx1.corp.redhat.com (8.13.1/8.13.1) with ESMTP id m8Q4cEZu006900; Fri, 26 Sep 2008 00:38:14 -0400 Received: from host0.dyn.jankratochvil.net (sebastian-int.corp.redhat.com [172.16.52.221]) by pobox.stuttgart.redhat.com (8.13.1/8.13.1) with ESMTP id m8Q4cCtJ022923; Fri, 26 Sep 2008 00:38:13 -0400 Received: from host0.dyn.jankratochvil.net (localhost [127.0.0.1]) by host0.dyn.jankratochvil.net (8.14.3/8.14.2) with ESMTP id m8Q4cB1f003796; Fri, 26 Sep 2008 06:38:11 +0200 Received: (from jkratoch@localhost) by host0.dyn.jankratochvil.net (8.14.3/8.14.2/Submit) id m8Q4cAvF003790; Fri, 26 Sep 2008 06:38:10 +0200 Date: Fri, 26 Sep 2008 05:03:00 -0000 From: Jan Kratochvil To: Joel Brobecker Cc: gdb-patches@sourceware.org, Tobias Burnus , Ulrich Weigand , Jim Blandy , jimb@codesourcery.com Subject: Re: [patch] static_kind -> bit0, bit1 [Re: [gdb] Fortran dynamic arrays] Message-ID: <20080926043810.GA1610@host0.dyn.jankratochvil.net> References: <20080818111120.GE16894@adacore.com> <200808181553.m7IFrG3w005270@d12av02.megacenter.de.ibm.com> <48A59B3C.9050801@net-b.de> <20080818111120.GE16894@adacore.com> <20080907115637.GA12939@host0.dyn.jankratochvil.net> <20080919060336.GD3651@adacore.com> <20080922151909.GA12274@host0.dyn.jankratochvil.net> <20080924191504.GA3613@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20080924191504.GA3613@adacore.com> User-Agent: Mutt/1.5.18 (2008-05-17) 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: 2008-09/txt/msg00511.txt.bz2 On Wed, 24 Sep 2008 21:15:04 +0200, Joel Brobecker wrote: > struct field has the following two components: > > /* For a function or member type, this is 1 if the argument is marked > artificial. Artificial arguments should not be shown to the > user. */ > unsigned int artificial : 1; > > /* This flag is zero for non-static fields, 1 for fields whose location > is specified by the label loc.physname, and 2 for fields whose location > is specified by loc.physaddr. */ > > unsigned int static_kind : 2; > > In practice, these are really what we call "discriminants" in Ada, which > means a value that we read in order to know what field to use in the > "loc" union. I agree "static_kind" is "discriminant" but not "artificial". Artificial is just additional flag existing only for FIELDS of type TYPE_CODE_FUNC or type TYPE_CODE_METHOD: FIELDs: artificial static_kind!=0 TYPE_CODE_ENUM no no TYPE_CODE_FUNC or TYPE_CODE_METHOD yes no TYPE_CODE_STRUCT yes(_vptr) yes(static field) Somehow I do not see how these two items are related - just each of them exists for some types and does not exist for some other types. Moreover the split into the big union gets more complicated when we merge them. Should I really rework the patch into the enum merging artificial+static_kind? > What does everyone think? I expect MAIN_TYPE may get reworked into a big union with per-type fields and all the bit-size alignment gets different etc. But otherwise I accept any decision on the layout of these. > > 2008-09-22 Jan Kratochvil > > > > Replace TYPE_ARRAY_{UPPER,LOWER}_BOUND_TYPE by a bit if {un,}defined. > > * gdb/c-typeprint.c (c_type_print_varspec_suffix), gdb/m2-typeprint.c ... > I am not sure about the reason why you listed all the filenames with > the gdb/ subdirectory in it, Sorry, a typo from the ChangeLog script http://sourceware.org/ml/gdb-patches/2006-08/msg00195.html , put there at least a warning now: warn "Extra directory" if $dfile=~m{(?:gdb|bfd|opcodes|libiberty)/}; > I do make sure to write each ChangeLog separately as shown above, though. > I noticed that you grouped all your changes, including your testsuite > changes into one CL entry. Sorry, also a typo, already written the split way many types: http://sourceware.org/ml/gdb-patches/2008-07/msg00145.html I put there the directory prefixes sometimes only in the CVS commits: http://sourceware.org/ml/gdb-cvs/2008-07/msg00048.html > > - retcode = f77_get_dynamic_lowerbound (type, &lower_bound); > > - > > - lower_bound_was_default = 0; > > - > > - if (retcode == BOUND_FETCH_ERROR) > > - fprintf_filtered (stream, "???"); > > - else if (lower_bound == 1) /* The default */ > > - lower_bound_was_default = 1; > > - else > > - fprintf_filtered (stream, "%d", lower_bound); > > - > > - if (lower_bound_was_default) > > - lower_bound_was_default = 0; > > - else > > - fprintf_filtered (stream, ":"); > > + lower_bound = f77_get_lowerbound (type); > > + if (lower_bound != 1) /* Not the default. */ > > + fprintf_filtered (stream, "%d:", lower_bound); > > Here, it looks like we're slightly modifying the behavior - if the lower > bound was undefined, then we're throwing an error when we used not to. That was a dead code. BOUND_FETCH_ERROR could never have been returned by f77_get_dynamic_lowerbound(): BOUND_SIMPLE returns BOUND_FETCH_OK. BOUND_CANNOT_BE_DETERMINED already error()s itself and never returns. Other BOUND_* cases were never set. Another question is if BOUND_CANNOT_BE_DETERMINED should error() as currently does or if it rather should return BOUND_FETCH_ERROR. But that decision is outside of the scope of my patch and changes the current GDB behavior. > > @@ -2719,14 +2690,13 @@ recursive_dump_type (struct type *type, > > } > > puts_filtered ("\n"); > > printfi_filtered (spaces, "length %d\n", TYPE_LENGTH (type)); > > - printfi_filtered (spaces, "upper_bound_type 0x%x ", > > - TYPE_ARRAY_UPPER_BOUND_TYPE (type)); > > - print_bound_type (TYPE_ARRAY_UPPER_BOUND_TYPE (type)); > > - puts_filtered ("\n"); > > - printfi_filtered (spaces, "lower_bound_type 0x%x ", > > - TYPE_ARRAY_LOWER_BOUND_TYPE (type)); > > - print_bound_type (TYPE_ARRAY_LOWER_BOUND_TYPE (type)); > > - puts_filtered ("\n"); > > + if (TYPE_CODE (type) == TYPE_CODE_ARRAY) > > + { > > + printfi_filtered (spaces, "upper bound undefined is %d\n", > > + TYPE_ARRAY_UPPER_BOUND_IS_UNDEFINED (type)); > > + printfi_filtered (spaces, "lower bound undefined is %d\n", > > + TYPE_ARRAY_LOWER_BOUND_IS_UNDEFINED (type)); > > + } > > I think this is only repetitive, and shouldn't be displayed. The > artificial field value will be displayed a little later, and that > should be good enough. No strong objection, though. As I hope the "artificial" field may start to exist only for some types I tried to make the patch more abstract to the specific implementation of TYPE_ARRAY_{UPPER,LOWER}_BOUND_IS_UNDEFINED. But no strong opinion on either way. Thanks, Jan