Re: [patch] static_kind -> bit0, bit1 [Re: [gdb] Fortran dynamic arrays]

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Wed, 24 Sep 2008 21:15:04 +0200, Joel Brobecker wrote:
> struct field has the following two components:
> 
>     /* For a function or member type, this is 1 if the argument is marked
>        artificial.  Artificial arguments should not be shown to the
>        user.  */
>     unsigned int artificial : 1;
> 
>     /* This flag is zero for non-static fields, 1 for fields whose location
>        is specified by the label loc.physname, and 2 for fields whose location
>        is specified by loc.physaddr.  */
> 
>     unsigned int static_kind : 2;
> 
> In practice, these are really what we call "discriminants" in Ada, which
> means a value that we read in order to know what field to use in the
> "loc" union.

I agree "static_kind" is "discriminant" but not "artificial".  Artificial is
just additional flag existing only for FIELDS of type TYPE_CODE_FUNC or type
TYPE_CODE_METHOD:
                                    FIELDs: artificial static_kind!=0
TYPE_CODE_ENUM                              no         no
TYPE_CODE_FUNC or TYPE_CODE_METHOD          yes        no
TYPE_CODE_STRUCT                            yes(_vptr) yes(static field)

Somehow I do not see how these two items are related - just each of them
exists for some types and does not exist for some other types.

Moreover the split into the big union gets more complicated when we merge
them.  Should I really rework the patch into the enum merging
artificial+static_kind?


> What does everyone think?

I expect MAIN_TYPE may get reworked into a big union with per-type fields and
all the bit-size alignment gets different etc.  But otherwise I accept any
decision on the layout of these.


> > 2008-09-22  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	Replace TYPE_ARRAY_{UPPER,LOWER}_BOUND_TYPE by a bit if {un,}defined.
> > 	* gdb/c-typeprint.c (c_type_print_varspec_suffix), gdb/m2-typeprint.c
...
> I am not sure about the reason why you listed all the filenames with
> the gdb/ subdirectory in it,

Sorry, a typo from the ChangeLog script
  http://sourceware.org/ml/gdb-patches/2006-08/msg00195.html
, put there at least a warning now:
  warn "Extra directory" if $dfile=~m{(?:gdb|bfd|opcodes|libiberty)/};


> I do make sure to write each ChangeLog separately as shown above, though.
> I noticed that you grouped all your changes, including your testsuite
> changes into one CL entry.

Sorry, also a typo, already written the split way many types:
http://sourceware.org/ml/gdb-patches/2008-07/msg00145.html
I put there the directory prefixes sometimes only in the CVS commits:
http://sourceware.org/ml/gdb-cvs/2008-07/msg00048.html


> > -      retcode = f77_get_dynamic_lowerbound (type, &lower_bound);
> > -
> > -      lower_bound_was_default = 0;
> > -
> > -      if (retcode == BOUND_FETCH_ERROR)
> > -	fprintf_filtered (stream, "???");
> > -      else if (lower_bound == 1)	/* The default */
> > -	lower_bound_was_default = 1;
> > -      else
> > -	fprintf_filtered (stream, "%d", lower_bound);
> > -
> > -      if (lower_bound_was_default)
> > -	lower_bound_was_default = 0;
> > -      else
> > -	fprintf_filtered (stream, ":");
> > +      lower_bound = f77_get_lowerbound (type);
> > +      if (lower_bound != 1)	/* Not the default.  */
> > +	fprintf_filtered (stream, "%d:", lower_bound);
> 
> Here, it looks like  we're slightly modifying the behavior - if the lower
> bound was undefined, then we're throwing an error when we used not to.

That was a dead code.  BOUND_FETCH_ERROR could never have been returned by
f77_get_dynamic_lowerbound():
BOUND_SIMPLE returns BOUND_FETCH_OK.
BOUND_CANNOT_BE_DETERMINED already error()s itself and never returns.
Other BOUND_* cases were never set.

Another question is if BOUND_CANNOT_BE_DETERMINED should error() as currently
does or if it rather should return BOUND_FETCH_ERROR.  But that decision is
outside of the scope of my patch and changes the current GDB behavior.


> > @@ -2719,14 +2690,13 @@ recursive_dump_type (struct type *type, 
> >      }
> >    puts_filtered ("\n");
> >    printfi_filtered (spaces, "length %d\n", TYPE_LENGTH (type));
> > -  printfi_filtered (spaces, "upper_bound_type 0x%x ",
> > -		    TYPE_ARRAY_UPPER_BOUND_TYPE (type));
> > -  print_bound_type (TYPE_ARRAY_UPPER_BOUND_TYPE (type));
> > -  puts_filtered ("\n");
> > -  printfi_filtered (spaces, "lower_bound_type 0x%x ",
> > -		    TYPE_ARRAY_LOWER_BOUND_TYPE (type));
> > -  print_bound_type (TYPE_ARRAY_LOWER_BOUND_TYPE (type));
> > -  puts_filtered ("\n");
> > +  if (TYPE_CODE (type) == TYPE_CODE_ARRAY)
> > +    {
> > +      printfi_filtered (spaces, "upper bound undefined is %d\n",
> > +			TYPE_ARRAY_UPPER_BOUND_IS_UNDEFINED (type));
> > +      printfi_filtered (spaces, "lower bound undefined is %d\n",
> > +			TYPE_ARRAY_LOWER_BOUND_IS_UNDEFINED (type));
> > +    }
> 
> I think this is only repetitive, and shouldn't be displayed. The
> artificial field value will be displayed a little later, and that
> should be good enough.  No strong objection, though.

As I hope the "artificial" field may start to exist only for some types
I tried to make the patch more abstract to the specific implementation of
TYPE_ARRAY_{UPPER,LOWER}_BOUND_IS_UNDEFINED.  But no strong opinion on either
way.



Thanks,
Jan