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

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

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  <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

Ahh maybe another nit, it would be nice if you could leave 
_initialize_f_language at the bottom of the file, for consistency.

Simon