Re: [PATCH 1/5] Fortran: Move calc_f77_array_dims.

[Simon Marchi <simon.marchi@polymtl.ca>]

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  <tim.wiederhake@intel.com>
> 
> 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