Re: RFA: fix crash in expression evaluation

[Joel Brobecker <brobecker@adacore.com>]

Hi Tom,

> 2008-10-02  Tom Tromey  <tromey@redhat.com>
> 
> 	* value.c (coerce_array): Use check_typedef.

Almost OK. Just a question...

> 2008-10-02  Tom Tromey  <tromey@redhat.com>
> 
> 	* gdb.base/pointers.exp: Add test.
> 	* gdb.base/pointers.c (k, S): New typedefs.
> 	(instance): New global.

This part looks fine and is OK to commit once we have clarified
my questions.

> @@ -1692,11 +1692,16 @@ coerce_ref (struct value *arg)
>  struct value *
>  coerce_array (struct value *arg)
>  {
> +  struct type *type;

Nit-picking. I think that the general convention here is to separate
local declarations from the rest of the block with an empty line.
I wonder if this is documented anywhere...

>    arg = coerce_ref (arg);
> +  type = check_typedef (value_type (arg));
>    if (current_language->c_style_arrays
> -      && TYPE_CODE (value_type (arg)) == TYPE_CODE_ARRAY)
> -    arg = value_coerce_array (arg);
> -  if (TYPE_CODE (value_type (arg)) == TYPE_CODE_FUNC)
> +      && TYPE_CODE (type) == TYPE_CODE_ARRAY)
> +    {
> +      arg = value_coerce_array (arg);
> +      type = check_typedef (value_type (arg));

I don't think the second line is useful, is it? type should necessarily
be a TYPE_CODE_PTR, if I understand value_coerce_array correctly. So
it goes from being a TYPE_CODE_ARRAY to a TYPE_CODE_PTR. In neither case
will it cause the check just below to be true.

> +    }
> +  if (TYPE_CODE (type) == TYPE_CODE_FUNC)
>      arg = value_coerce_function (arg);
>    return arg;
>  }

Honestly, I think that the code is poorly written. How about using
a case statement or at least a if/else if sequence. Would something
like this work?

    struct type *type;

    arg = coerce_ref (arg);
    type = check_typedef (value_type (arg))

    switch (TYPE_CODE (type))
      {
        case TYPE_CODE_ARRAY:
          if (current_language->c_style_arrays)
            arg = value_coerce_array (arg);
          break;
        case TYPE_CODE_FUNC:
          arg = value_coerce_function (arg);
          break
      }
    return arg;

What do you think?

-- 
Joel

:REVIEWMAIL: