From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23927 invoked by alias); 16 May 2003 15:25:21 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 23912 invoked from network); 16 May 2003 15:25:21 -0000 Received: from unknown (HELO crack.them.org) (146.82.138.56) by sources.redhat.com with SMTP; 16 May 2003 15:25:21 -0000 Received: from nevyn.them.org ([66.93.61.169] ident=mail) by crack.them.org with asmtp (Exim 3.12 #1 (Debian)) id 19Gh5a-0004nE-00; Fri, 16 May 2003 10:25:42 -0500 Received: from drow by nevyn.them.org with local (Exim 3.36 #1 (Debian)) id 19Gh55-0001ae-00; Fri, 16 May 2003 11:25:11 -0400 Date: Fri, 16 May 2003 15:25:00 -0000 From: Daniel Jacobowitz To: David Lecomber Cc: gdb-patches@sources.redhat.com Subject: Re: [PATCH] Improvements to Fortran support Message-ID: <20030516152511.GA5866@nevyn.them.org> Mail-Followup-To: David Lecomber , gdb-patches@sources.redhat.com References: <20030115213240.A17967@streamline-computing.com> <20030516122103.A31934@streamline-computing.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20030516122103.A31934@streamline-computing.com> User-Agent: Mutt/1.5.1i X-SW-Source: 2003-05/txt/msg00279.txt.bz2 On Fri, May 16, 2003 at 12:21:03PM +0100, David Lecomber wrote: > Reposting as I now have the copyright assignment in place.. > > Fortran arrays are presently allocated in their entirety and then the > correct element is pulled out. This (a) doesn't scale, (b) doesn't > work if the array is a parameter to a subroutine and supplied with a > (*) in the dimensions (GDB runs out of memory doing malloc( -1 )..) > > The fix here makes the behaviour the same as the code for doing C > arrays.. Thanks for the patch and your patience. I have a couple of small comments on your changes. First of all, when submitting a patch, please include a standard ChangeLog entry - even for small patches like this one, it makes it easier to see at a glance what's going on and where. For this patch it would look something like: 2003-05-16 David Lecomber * eval.c (evaluate_subexp_standard): Correct handling for Fortran multidimensional array subscripts. Secondly, some nits about formatting: > ! /* Construct a value node with the value of the offset */ > ! /* lower will get subtracted off in value_subscript, hence add it here */ Should be: /* Construct a value node with the value of the offset. LOWER will get subtracted off in value_subscript, hence add it here. */ (i.e. full sentences, two spaces after a full stop.) And, my only substantive comments: - Could you explain why this makes a difference? You change value_ind (value_add (value_coerce_array (), idx))) into value_subscript (array, idx). value_subscript will call value_coerce_array, then value_add and value_ind on the result. - How does f77_get_dynamic_lowerbound end up being called from value_subscript? I couldn't figure it out, which says that either I woke up denser than usual this morning or a better comment is in order. > *** eval.c Sun Dec 15 22:29:59 2002 > --- eval.c Sun Dec 15 22:28:41 2002 > *************** evaluate_subexp_standard (struct type *e > *** 1383,1392 **** > offset_item = > array_size_array[i] * offset_item + subscript_array[i]; > > - /* Construct a value node with the value of the offset */ > - > - arg2 = value_from_longest (builtin_type_f_integer, offset_item); > - > /* Let us now play a dirty trick: we will take arg1 > which is a value node pointing to the topmost level > of the multidimensional array-set and pretend > --- 1383,1388 ---- > *************** evaluate_subexp_standard (struct type *e > *** 1395,1401 **** > returns the correct type value */ > > VALUE_TYPE (arg1) = tmp_type; > ! return value_ind (value_add (value_coerce_array (arg1), arg2)); > } > > case BINOP_LOGICAL_AND: > --- 1391,1405 ---- > returns the correct type value */ > > VALUE_TYPE (arg1) = tmp_type; > ! > ! f77_get_dynamic_lowerbound (tmp_type, &lower); > ! > ! /* Construct a value node with the value of the offset */ > ! /* lower will get subtracted off in value_subscript, hence add it here */ > ! > ! arg2 = value_from_longest (builtin_type_f_integer, offset_item + lower); > ! > ! return value_subscript(arg1, arg2); > } > > case BINOP_LOGICAL_AND: > > > David > > -- Daniel Jacobowitz MontaVista Software Debian GNU/Linux Developer