Re: [PATCH v4 01/13] vla: introduce new bound type abstraction adapt uses

[Joel Brobecker <brobecker at adacore dot com>]

> The rational behind this patch is to get started to implement the feature
> described in dwarf4 standard (2.19) Static and Dynamic Values of Attributes.
> It adds new BOUND_PROP to store either a constant, exprloc, or reference to
> describe an upper-/lower bound of a subrange. Other than that no new features
> are introduce.
> 
> 2013-10-18  Sanimir Agovic  <sanimir.agovic@intel.com>
>             Keven Boell  <keven.boell@intel.com>
> 
> 	* dwarf2read.c (read_subrange_type): Use struct bound_prop for
> 	declaring high/low bounds and change uses accordingly. Call
> 	create_range_type_1 instead of create_range_type.
> 	* gdbtypes.c (create_range_type_1): New function.
> 	(create_range_type): Convert bounds into struct bound_prop and pass
> 	them to create_range_type_1.
> 	* gdbtypes.h (struct bound_prop): New struct.
> 	(create_range_type_1): New function prototype.
> 	(struct range_bounds): Use struct bound_prop instead of LONGEST for
> 	high/low bounds. Remove low_undefined/high_undefined and adapt all uses.
> 	(TYPE_LOW_BOUND,TYPE_HIGH_BOUND): Adapt macros to refer to the static
> 	part of the bound.
> 	* parse.c (follow_types): Set high bound kind to BOUND_UNDEFINED.

Just a suggestion, which you may choose to ignore.

I think that the _1 suffix is usually used when the function performs
the private portion of a more public routine.  But in this case,
create_range_type_1 is meant to be a public routine, and the _1
suffix is not very explicit.  IMO, what would be ideal would be to
rename the current create_range_type into "create_static_range_type",
and then make create_range_type_1 the new create_range_type. I checked
the GDB tree, and there aren't that many calls to update. If people
prefer, I can even take care of that myself once the patche series
has gone in. Otherwise, another compromise solution is to rename
create_range_type_1 to create_range_type_full (for instance).

> +/* Used to store a dynamic property.  */
> +
> +struct dynamic_prop
> +{
> +  /* Determine which field of the union dynamic_prop.data is used.  */
> +  enum
> +  {
> +    PROP_UNDEFINED,
> +    PROP_CONST,
> +    PROP_LOCEXPR,
> +    PROP_LOCLIST
> +  } kind;
> +
> +  /* Storage for dynamic or static value.  */
> +  union data
> +  {
> +    LONGEST const_val;
> +    void *baton;
> +  } data;

Would you mind documenting each enumeration and union field?

> +#define TYPE_LOW_BOUND(range_type) \
> +  TYPE_RANGE_DATA(range_type)->low.data.const_val
> +#define TYPE_HIGH_BOUND(range_type) \
> +  TYPE_RANGE_DATA(range_type)->high.data.const_val
>  #define TYPE_LOW_BOUND_UNDEFINED(range_type) \
> -   TYPE_RANGE_DATA(range_type)->low_undefined
> +  (TYPE_RANGE_DATA(range_type)->low.kind == PROP_UNDEFINED)
>  #define TYPE_HIGH_BOUND_UNDEFINED(range_type) \
> -   TYPE_RANGE_DATA(range_type)->high_undefined
> +  (TYPE_RANGE_DATA(range_type)->high.kind == PROP_UNDEFINED)
> +#define TYPE_HIGH_BOUND_KIND(range_type) \
> +  TYPE_RANGE_DATA(range_type)->high.kind
> +#define TYPE_LOW_BOUND_KIND(range_type) \
> +  TYPE_RANGE_DATA(range_type)->low.kind

For the record, I considered the idea of adding asserts in there,
in order to get an internal error instead of an odd bug when accessing
the wrong field. But this requires us making these macros read-only
accessors, rather than read-write. A quick experiment showed that
some units are using them to write some fields, and so we would need
to audit that first. It's a desirable change on its own, IMO, regardless
of whether we thinking adding the assert is desirable or not, but I don't
want to put the burden on this patch series, which seems already quite
sizeable on its own already.

-- 
Joel