Re: [PATCH 5/6] Respect `set print array-indexes' with Fortran arrays

["Maciej W. Rozycki" <macro@embecosm.com>]

On Wed, 15 Dec 2021, Andrew Burgess wrote:

> > Index: src/gdb/f-array-walker.h
> > ===================================================================
> > --- src.orig/gdb/f-array-walker.h
> > +++ src/gdb/f-array-walker.h
> > @@ -138,7 +138,8 @@ struct fortran_array_walker_base_impl
> >       true only when this is the last element that will be processed in
> >       this dimension.  */
> >    void process_dimension (std::function<void (struct type *, int, bool)> walk_1,
> > -			  struct type *elt_type, LONGEST elt_off, bool last_p)
> > +			  struct type *elt_type, LONGEST elt_off,
> > +			  struct type *, LONGEST, bool last_p)
> 
> I'd like to see the new arguments given names, and the comment
> extended to explain what they're for, given this is the base
> implementation from which others inherit it seems like this is the
> obvious place someone would look to figure this sort of thing out.

 That is the proper C++ syntax for unused parameters, preferred for GCC, 
the style of which we follow according to our wiki, which I double-checked 
on this occasion as I wrote this code, as far as C++ usage is concerned, 
over `__attribute__ ((__unused__))' since the switch to C++.  And I wasn't 
aware we chose to use `-Wno-unused' instead, which may not be as clean as 
the standard C++ syntax.  I think this divergence from the GCC C++ coding 
style needs to be clearly documented in our wiki.

 I've added the missing argument names then here and elsewhere (a viable 
alternative would be to have the argument names commented out).
  
> > @@ -223,6 +225,8 @@ class fortran_array_walker
> >      if (m_nss != m_ndimensions)
> >        {
> >  	struct type *subarray_type = TYPE_TARGET_TYPE (check_typedef (type));
> > +	gdb_assert (range_type->code () == TYPE_CODE_RANGE);
> 
> I think this assert should move up to immediately after we fetch
> range_type, I'm pretty sure that if range_type isn't TYPE_CODE_RANGE
> then the call to get_discrete_bounds will have already gone wrong.

 Well, I have checked and `get_discrete_bounds' looks prepared to several 
other types, specifically: TYPE_CODE_ENUM, TYPE_CODE_BOOL, TYPE_CODE_INT 
and TYPE_CODE_CHAR, so this assertion is only for TYPE_TARGET_TYPE really, 
so for clarity I've left it right before the invocation of the latter 
macro.

> > @@ -260,7 +264,8 @@ class fortran_array_walker
> >  		elt_type = resolve_dynamic_type (elt_type, {}, e_address);
> >  	      }
> >  
> > -	    m_impl.process_element (elt_type, elt_off, (i == upperbound));
> > +	    m_impl.process_element (elt_type, elt_off, range_type, i,
> > +				    i == upperbound);
> 
> This seems a little weird, you're passing the range_type through to
> the index_type parameter?  Is this really what you mean?

 It's a stupid oversight on my side; this should have been `index_type' in 
both legs of the conditional.  I have adjusted code accordingly (though 
it's now gone owing to the change described below).

> Assuming you mean to pass TYPE_TARGET_TYPE(range_type) here, then the
> index_type ends up being passed through to both process_element and
> process_dimension.  You then end up placing the index_type into a
> member variable within the m_impl.  I wonder if it would be better to
> pass the index_type to start_dimension, and stash it there, then we'd
> only need to pass the index value through maybe?  Or, maybe there
> would be problems with handling multi-dimensional arrays?  I guess
> it's pretty hard to actually uncover bugs in this area as the
> index_type is almost always the same I think....

 I chewed your thoughts over and realised that if we pass `index_type' via 
`start_dimension' then not only some code complication is removed, but if 
we stash it per-dimension, then we'll have no issue if we ever need to use 
different types for individual dimensions.  And the rank of an array is at 
most seven in Fortran, so it's not like it will consume a lot of memory.

 See v2 for how the result looks like.

  Maciej