This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: RFA: fix crash in expression evaluation
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Tom Tromey <tromey at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Wed, 29 Oct 2008 20:54:43 -0700
- Subject: Re: RFA: fix crash in expression evaluation
- References: <m3od22hpue.fsf@fleche.redhat.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: