On Thursday, September 30, 2010 8:56:34 pm Joel Brobecker wrote:
> > 	* eval.c (evaluate_subexp_standard) <UNOP_POSTINCREMENT,
> > 	UNOP_POSTDECREMENT>: Copy arg1 to prevent returning a lvalue.
> 
> Would you mind if we treated this as a separate patch, on top of
> the current patch? I'd also like to add a test that verifies this,
> hopefully not involving vector types.

Ok, good point - here we go:
http://sourceware.org/ml/gdb-patches/2010-10/msg00012.html

> You also seem to do cleanups at the same time, which is OK. But
> I'd rather they be submitted separately. It's not all that bad,
> but it does tend to pollute a bit the meat of the change you are
> trying to make.  Fortunately, tools such as git make it really easy
> to manage (I know Dan an Pedro use something different, but I just
> can't remember the name of that tool).

Thanks for the advice. The new patch changes only what I think is absolutely 
necessary in order to implement the wanted functionality.

> > +  if (TYPE_CODE (type) == TYPE_CODE_ARRAY && TYPE_VECTOR (type))
> > 
> >      {
> > 
> > -      return value_from_longest (type, value_as_long (arg1));
> > +      val = allocate_value (type);
> > +      memcpy (value_contents_raw (val), value_contents (arg1),
> > +	      TYPE_LENGTH (type));
> > 
> >      }
> 
> I'd rather you put the check for vector types at the end of the
> if/elsif sequence. I think that vector types are going to be less
> common than integers, floats et al.  This is true for all the changes
> you made.

Sounds reasonable. Fixed.

> One question: Is it possible to have a non-array vector type?
> In other words, can we just check for TYPE_VECTOR (type) instead
> of TYPE_CODE (type) and TYPE_VECTOR (type)?

No, not that I'm aware of. Even a GNU Vector with a single element only is an 
array underneath. My understanding is that querying the flag_vector is only 
legal if the type is an array. Historically vectors have been treated as 
arrays and there were only a few cases within GDB where arrays and vector were 
handled differently. There was no need to introduce a new type code. The more I 
do with vectors within GDB the more I think it would be nice to have such a 
distinct type code.

> Please move the assignment to `val' one line below (empty line after
> the declarations, and no need for an empty line between the two
> statements).

Ok.

> Unnecessary empty line?

Yep. Thanks.

> > @@ -421,7 +421,8 @@ value_cast (struct type *type, struct va
> > 
> >      }
> >    
> >    if (current_language->c_style_arrays
> > 
> > -      && TYPE_CODE (type2) == TYPE_CODE_ARRAY)
> > +      && TYPE_CODE (type2) == TYPE_CODE_ARRAY
> > +      && ! TYPE_VECTOR (type2))
> > 
> >      arg2 = value_coerce_array (arg2);
> 
> You forgot to mention this change in you ChangeLog, and I don't
> understand why it's necessary? Can you elaborate? (and add a test
> if appropriate)

Good catch. I thought that value_cast should not call value_coerce_array for 
vectors. You probably don't want a value which is a pointer to the first 
element in that case because vectors aren't arrays. Since this has nothing 
particularly to do with what the patch intended to improve I removed this 
chunk.

> Empty line after the declarations.
Fixed.

This new patch depends on the lval-fix posted here:
http://sourceware.org/ml/gdb-patches/2010-10/msg00029.html
Thanks for looking into it.

Regards
Ken Werner