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

[Joel Brobecker <brobecker@adacore.com>]

> Here is the patch.

Argh, it took me a long time to break through the patch because
I had missed the fact that the upper-bound was indeed set to
BOUND_CANNOT_BE_DETERMINED once. I think you were pretty clear
about this in your previous message, but my (hopefully then)
fried brain didn't register. I'm sorry I missed that somehow,
because it changes slightly my point of view. To make it up to you,
I will try to help as much as I can.

Anyway, so the patch is using the TYPE_FIELD_ARTIFICIAL bit to
detect range types where a given bound value cannot be determined.
The patch as it is would work, and I wouldn't object to having it
checked in as a small step forward (with the agreement from at least
one other maintainer). However, I think we can implement this slightly
differently to make it more general.

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 propose that, instead of re-using the "artificial" flag
for undefined bounds, we merge the two components "artificial" and
"static_kind" together into a 3-bit field whose value would be an enum
that tells us what field to use for our field loc.

Because this field would be 3-bit long, it would allow us to have 7
different possible fields, in addition to the case where the bound
is undefined or the argument is artificial.  That would leave more
than enough room to then add a DWARF block as a possible field loc.

What does everyone think?

Some comments on your patch, that I did review pretty carefully:

> 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
> 	(m2_array), gdb/p-typeprint.c (pascal_type_print_varspec_prefix),
> 	gdb/valops.c (value_cast), gdb/varobj.c (c_number_of_children): Replace
> 	TYPE_ARRAY_UPPER_BOUND_TYPE compared to BOUND_CANNOT_BE_DETERMINED by
> 	TYPE_ARRAY_UPPER_BOUND_IS_UNDEFINED.
[...]

I am not sure about the reason why you listed all the filenames with
the gdb/ subdirectory in it, but the gdb/ subdir has its own ChangeLog,
so it shouldn't be part of the file name. Perhaps it's because you wanted
to make it clear that the changes are in the gdb/ directory.  Usually,
it's obvious enough that I don't bother.  When I make changes to more
than one directory such that it will cause more than one ChangeLog to
be updated, I sometimes write something like this:

    gdb/
    2008-09-22  Joel Brobecker <....>
    [...]

    gdb/testsuite/
    2008-09-22  Joel Brobecker <...>

Most of the time, I don't even bother, as it is clear enough.  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.

> -      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.
I think that would be unfriendly if this issue is caused by bad debug
info. Should we check that the lower bound is not undefined first, and
print "???" if it is?

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

-- 
Joel