This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] GNU vector unop support


> 2010-09-17  Ken Werner  <ken.werner@de.ibm.com>
> 
> 	* 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.

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).

> 	* valarith.c (value_pos, value_neg, value_complement): Handle
> 	vector types.
> 	* valops.c (value_one): Likewise.

Seems mostly OK. Just a few questions and nits.

> testsuite/ChangeLog:
> 
> 2010-09-17  Ken Werner  <ken.werner@de.ibm.com>
> 
> 	* gdb.base/gnu_vector.exp: Add unary operator tests.

This looks OK.

> +  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.

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)?

> +  else if (TYPE_CODE (type) == TYPE_CODE_DECFLOAT)
>      {
> -      struct value *val = allocate_value (type);
>        int len = TYPE_LENGTH (type);
>        gdb_byte decbytes[16];  /* a decfloat is at most 128 bits long */
> +      val = allocate_value (type);
>  
>        memcpy (decbytes, value_contents (arg1), len);

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).

> @@ -1715,33 +1734,53 @@ value_neg (struct value *arg1)
>  	decbytes[0] = decbytes[0] | 0x80;
>  
>        memcpy (value_contents_raw (val), decbytes, len);
> -      return val;
> +

Unnecessary empty line?

> @@ -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)

> +  if (TYPE_CODE (type1) == TYPE_CODE_ARRAY && TYPE_VECTOR (type1))
> +    {
> +      struct type *eltype = check_typedef (TYPE_TARGET_TYPE (type1));
> +      int i, n = TYPE_LENGTH (type1) / TYPE_LENGTH (eltype);
> +      struct value *tmp;
> +      val = allocate_value (type);
> +      for (i = 0; i < n; i++)

Empty line after the declarations.

-- 
Joel


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]