From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 114134 invoked by alias); 15 Sep 2017 20:28:29 -0000 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 Received: (qmail 114122 invoked by uid 89); 15 Sep 2017 20:28:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 15 Sep 2017 20:28:27 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id v8FKSKmD007543 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Fri, 15 Sep 2017 16:28:25 -0400 Received: by simark.ca (Postfix, from userid 112) id BA09A1ECEE; Fri, 15 Sep 2017 16:28:20 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 5BCD11EA9D; Fri, 15 Sep 2017 16:28:10 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Fri, 15 Sep 2017 20:28:00 -0000 From: Simon Marchi To: Tim Wiederhake Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/5] Fortran: Move calc_f77_array_dims. In-Reply-To: References: <1505134663-29374-1-git-send-email-tim.wiederhake@intel.com> <1505134663-29374-2-git-send-email-tim.wiederhake@intel.com> Message-ID: X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Fri, 15 Sep 2017 20:28:20 +0000 X-IsSubscribed: yes X-SW-Source: 2017-09/txt/msg00426.txt.bz2 On 2017-09-15 22:21, Simon Marchi wrote: > Hi Tim, > > The patch is ok, with some nits I noted below. > > On 2017-09-11 14:57, Tim Wiederhake wrote: >> 2017-09-11 Tim Wiederhake >> >> gdb/ChangeLog: >> * eval.c (evaluate_subexp_standard): Use new function name. >> (calc_f77_array_dims): Move ... >> * f-lang.c (f77_get_array_dims): ... here. Constify argument. Make >> NULL check explicit. >> * f-lang.h (calc_f77_arra_dims): Rename to... >> (f77_get_array_dims): ... this. Add comment. >> * f-valprint.c (f77_print_array): Use new function name. >> >> --- >> gdb/eval.c | 21 +-------------------- >> gdb/f-lang.c | 16 ++++++++++++++++ >> gdb/f-lang.h | 4 +++- >> gdb/f-valprint.c | 2 +- >> 4 files changed, 21 insertions(+), 22 deletions(-) >> >> diff --git a/gdb/eval.c b/gdb/eval.c >> index 24f32f8..7a808a0 100644 >> --- a/gdb/eval.c >> +++ b/gdb/eval.c >> @@ -2336,7 +2336,7 @@ evaluate_subexp_standard (struct type >> *expect_type, >> if (nargs > MAX_FORTRAN_DIMS) >> error (_("Too many subscripts for F77 (%d Max)"), >> MAX_FORTRAN_DIMS); >> >> - ndimensions = calc_f77_array_dims (type); >> + ndimensions = f77_get_array_dims (type); >> >> if (nargs != ndimensions) >> error (_("Wrong number of subscripts")); >> @@ -3266,22 +3266,3 @@ parse_and_eval_type (char *p, int length) >> error (_("Internal error in eval_type.")); >> return expr->elts[1].type; >> } >> - >> -int >> -calc_f77_array_dims (struct type *array_type) >> -{ >> - int ndimen = 1; >> - struct type *tmp_type; >> - >> - if ((TYPE_CODE (array_type) != TYPE_CODE_ARRAY)) >> - error (_("Can't get dimensions for a non-array type")); >> - >> - tmp_type = array_type; >> - >> - while ((tmp_type = TYPE_TARGET_TYPE (tmp_type))) >> - { >> - if (TYPE_CODE (tmp_type) == TYPE_CODE_ARRAY) >> - ++ndimen; >> - } >> - return ndimen; >> -} >> diff --git a/gdb/f-lang.c b/gdb/f-lang.c >> index 903cfd1..77b759b 100644 >> --- a/gdb/f-lang.c >> +++ b/gdb/f-lang.c >> @@ -370,3 +370,19 @@ _initialize_f_language (void) >> { >> f_type_data = gdbarch_data_register_post_init >> (build_fortran_types); >> } >> + >> +/* See f-lang.h. */ >> + >> +int >> +f77_get_array_dims (const struct type *array_type) >> +{ >> + if ((TYPE_CODE (array_type) != TYPE_CODE_ARRAY)) >> + error (_("Can't get dimensions for a non-array type")); >> + >> + int ndimen = 0; >> + for (; array_type != NULL; array_type = TYPE_TARGET_TYPE >> (array_type)) >> + if (TYPE_CODE (array_type) == TYPE_CODE_ARRAY) >> + ndimen += 1; > > ndimen++; ? > > Just a question (just showing off my ignorance of fortran): don't you > want to stop looping when you reach a type that is a non-array? I > don't know which types in fortran can have a target, but is it > possible to have something like this? > > array type -> array type -> other type -> array type > > The code above would give a dimension of 3, is it what we want? I > would have thought that the "top-level" array has a dimension of 2, > but its elements is of type "other type". > >> + >> + return ndimen; >> +} >> diff --git a/gdb/f-lang.h b/gdb/f-lang.h >> index 5633b41..cfe667b 100644 >> --- a/gdb/f-lang.h >> +++ b/gdb/f-lang.h >> @@ -55,7 +55,9 @@ extern int f77_get_lowerbound (struct type *); >> >> extern void f77_get_dynamic_array_length (struct type *); >> >> -extern int calc_f77_array_dims (struct type *); >> +/* Calculate the number of dimensions of an array. Expects >> ARRAY_TYPE to be >> + * the type of an array. */ > > Nit: remove the star at the beginning of the second line. > > I would suggest this wording: Calculate the number of dimensions of an > array of type ARRAY_TYPE. > >> +extern int f77_get_array_dims (const struct type *array_type); >> >> >> /* Fortran (F77) types */ >> diff --git a/gdb/f-valprint.c b/gdb/f-valprint.c >> index 8fc894a..59d1a2f 100644 >> --- a/gdb/f-valprint.c >> +++ b/gdb/f-valprint.c >> @@ -180,7 +180,7 @@ f77_print_array (struct type *type, const gdb_byte >> *valaddr, >> int ndimensions; >> int elts = 0; >> >> - ndimensions = calc_f77_array_dims (type); >> + ndimensions = f77_get_array_dims (type); >> >> if (ndimensions > MAX_FORTRAN_DIMS || ndimensions < 0) >> error (_("\ > > Thanks, > > Simon Ahh maybe another nit, it would be nice if you could leave _initialize_f_language at the bottom of the file, for consistency. Simon