On Tuesday, August 10, 2010 09:06:14 pm Tom Tromey wrote:
> >>>>> "Ken" == Ken Werner <ken@linux.vnet.ibm.com> writes:
> Ken> This patch implements some binary operations for GNU vectors by
> Ken> traversing through the vector and calling scalar_binop (the former
> Ken> value_binop) element wise.
> 
> Thanks for tackling this.
> 
> This patch looks good to me.  I noticed a few nits, and one slightly
> more serious problem.

Thank you for reviewing the patch.

> Ken>  struct value *
> Ken> -value_binop (struct value *arg1, struct value *arg2, enum exp_opcode
> op) Ken> +scalar_binop (struct value *arg1, struct value *arg2, enum
> exp_opcode op)
> 
> Should be static.
> 
> Ken> +struct value *
> Ken> +vector_binop (struct value *val1, struct value *val2, enum exp_opcode
> op)
> 
> Likewise.

I agree. Those functions should be file static since value.h only exposes 
value_binop.

> Ken> +  if (TYPE_CODE (eltype1) != TYPE_CODE (eltype2))
> Ken> +    error (_("The vectors have different types"));
> Ken> +
> Ken> +  elsize =  TYPE_LENGTH (eltype1);
> 
> Extra space after the "=".

Fixed.

> Ken> +  n = TYPE_LENGTH (type1) / elsize;
> Ken> +  val = allocate_value (type1);
> Ken> +
> Ken> +  mark = value_mark ();
> Ken> +  for (i = 0; i < n; i++)
> Ken> +    {
> Ken> +      tmp = value_binop (value_subscript (val1, i),
> Ken> +			 value_subscript (val2, i), op);
> Ken> +      memcpy (value_contents_writeable (val) + i * elsize,
> Ken> +	      value_contents_all (tmp),
> Ken> +	      elsize);
> 
> It seems to me that this could use some more checking.
> 
> The TYPE_CODEs could be equal but still give the wrong answer here.  For
> example, 'int' and 'long' will have the same TYPE_CODE, but different
> sizes.  Depending on the order of the addition, this could cause the
> memcpy to overflow the destination buffer.

I've added checks for:
 - the length of the target types
 - the unsigned flag of the target types
 - and the size of the vectors as Jan suggested.

> Ken> +set testfile "gnu_vector"
> Ken> +set srcfile ${testfile}.c
> Ken> +set binfile ${objdir}/${subdir}/${testfile}
> Ken> +
> Ken> +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}"
> executable {debug}] != "" } { Ken> +    untested "Couldn't compile
> ${srcfile}"
> Ken> +    return -1
> Ken> +}
> Ken> +
> Ken> +if [get_compiler_info ${binfile}] {
> Ken> +    return -1
> Ken> +}
> Ken> +
> Ken> +gdb_exit
> Ken> +gdb_start
> Ken> +gdb_reinitialize_dir $srcdir/$subdir
> Ken> +gdb_load $binfile
> 
> I think most of this (except the get_compiler_info bit) can be replaced
> with prepare_for_testing.

OK, done - thanks for the hint. In addition I've added some tests that check 
the various error conditions.

Attached is the revised patch.

Regards
Ken