Re: [PATCH V2 1/3] fort_dyn_array: Enable dynamic member types inside a structure.

[Joel Brobecker <brobecker at adacore dot com>]

Just a few minor nits that I happen to see while glancing at the patch:

> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -2064,7 +2064,8 @@ resolve_dynamic_struct (struct type *type,
>  
>        pinfo.type = check_typedef (TYPE_FIELD_TYPE (type, i));
>        pinfo.valaddr = addr_stack->valaddr;
> -      pinfo.addr = addr_stack->addr;
> +      pinfo.addr = addr_stack->addr
> +              + (TYPE_FIELD_BITPOS (resolved_type, i) / TARGET_CHAR_BIT);

The GCS recommends in that case to use an extra pair of parens.
The purpose is to help code formatters align the code properly.

      pinfo.addr
        = (addr_stack->addr
           + (TYPE_FIELD_BITPOS (resolved_type, i) / TARGET_CHAR_BIT));

> +  /* The length of a type won't change for fortran, but it does for C and Ada.
> +     For fortran the size of dynamic fields might change over time but not the
> +     type length of the structure.  If we would adapt it we run into problems
> +     when calculating the element offset for arrays of structs.  */

May I suggest the following rephrasing, which sounds a litle better
to me?

"If we adapt it, we run into problems [...]"

> +/* Remove dynamic property from TYPE in case it exist.  */

"exists"

> +void
> +remove_dyn_prop (enum dynamic_prop_node_kind prop_kind,
> +                 struct type *type)
> +{
> +  struct dynamic_prop_list *prev_node, *curr_node;
> +
> +  curr_node = TYPE_DYN_PROP_LIST (type);
> +  prev_node = NULL;
> +
> +  while (NULL != curr_node)
> +    {
> +      if (curr_node->prop_kind == prop_kind)
> +	{
> +	  /* Upadate the linked list but don't free anything.

"Update"

> +      /* Internal variables which are created from values with a dynamic location
> +         don't need the location property of the origin anymore.
> +         Remove the location property in case it exist.  */
> +      remove_dyn_prop (DYN_PROP_DATA_LOCATION, value_type (new_data.value));

I am also wondering why it makes a difference to remove it, and
it would be useful to explain it here.

-- 
Joel