From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29267 invoked by alias); 26 Sep 2008 22:12:14 -0000 Received: (qmail 29252 invoked by uid 22791); 26 Sep 2008 22:12:14 -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, 26 Sep 2008 22:11:23 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id D67BC2A96D8; Fri, 26 Sep 2008 18:11:21 -0400 (EDT) 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 yvb9m47yYlvE; Fri, 26 Sep 2008 18:11:21 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 3D8662A96AA; Fri, 26 Sep 2008 18:11:21 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 2918CE7ACD; Fri, 26 Sep 2008 15:11:19 -0700 (PDT) Date: Fri, 26 Sep 2008 22:12:00 -0000 From: Joel Brobecker To: Jan Kratochvil 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: <20080926221119.GC3814@adacore.com> 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> <20080926043810.GA1610@host0.dyn.jankratochvil.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20080926043810.GA1610@host0.dyn.jankratochvil.net> 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-09/txt/msg00527.txt.bz2 > 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: Yes, I don't know why I was thinking that artificial was a discriminant (meaning "no location"). > 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? No that won't be necessary. I agree with Daniel that we should go with your patch for now. Improving the clarity of the discriminant can be done separately. > 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)/}; No problem, please just remember to fix these before you commit. > > > + 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. Hmmm, you are right! So that piece was fine too. > 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. Absolutely. > > > + 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. That's only my opinion too, and there is no real reason why my opinion should count too. But to me, this function does a low-level dump of our type structure, and thus should follow closely the implementation. If we start to introduce a higher-level view of this type, I think it can potentially be confusing. For now, I would prefer to leave that part out. Your patch is approved with the few corrections I mentioned. Thank you. -- Joel