Re: [RFA] BINOP_DIV and ptyp command

[Daniel Jacobowitz <drow at false dot org>]

On Wed, Jan 30, 2008 at 09:28:52AM -0800, Doug Evans wrote:
> +	  /* If EVAL_AVOID_SIDE_EFFECTS and we're dividing by zero,
> +	     fudge arg2 to avoid division-by-zero, the caller is
> +	     (theoretically) only looking for the type of the result.  */
> +	  if (noside == EVAL_AVOID_SIDE_EFFECTS
> +	      /* ??? Do we really want to test for BINOP_MOD here?
> +		 The implementation of value_binop gives it a well-defined
> +		 value.  */
> +	      && (op == BINOP_DIV
> +		  || op == BINOP_INTDIV
> +		  || op == BINOP_REM
> +		  || op == BINOP_MOD)
> +	      && value_logical_not (arg2))
> +	    {
> +	      struct value *v_one, *retval;
> +
> +	      v_one = value_one (value_type (arg2), not_lval);
> +	      retval = value_binop (arg1, v_one, op);
> +	      value_free (v_one);
> +	      return retval;
> +	    }

Can't call value_free here.  That's just a call to free, and values
are kept in a linked list; see value_release and value_free_to_mark.
In general, it's fine to leak values unless you're in a loop.  If your
caller cares, it will use value_free_to_mark to clean up; otherwise it
will happen at the top level.

value_one has the same issue.

> +  else if (is_integral_type (type1))
> +    {
> +      /* Perform integral promotion for ANSI C/C++.
> +	 If not appropropriate for any particular language it needs to
> +	 supply its own la_unop_result_type function.  */

No la_unop_result_type in this version (which I like best of the
several you posted).

> +      switch (current_language->la_language)
> +	{
> +	case language_c:
> +	case language_cplus:
> +	case language_asm:
> +	  /* ??? language_objc? */
> +	  /* Perform ANSI/ISO-C promotions.
> +	     If only one type is float, use its type.
> +	     Otherwise use the bigger type.  */
> +	  if (TYPE_CODE (type1) != TYPE_CODE_FLT)
> +	    return type2;
> +	  else if (TYPE_CODE (type2) != TYPE_CODE_FLT)
> +	    return type1;
> +	  else
> +	    return (TYPE_LENGTH (type1) > TYPE_LENGTH (type2)) ? type1 : type2;
> +
> +	default:
> +	  /* For other languages the result type is unchanged from 6.7 for
> +	     backward compatibility.
> +	     If either arg was long double, make sure that value is also long
> +	     double.  */
> +	  if (TYPE_LENGTH (type1) * 8 > gdbarch_double_bit (current_gdbarch)
> +	      || TYPE_LENGTH (type2) * 8 > gdbarch_double_bit (current_gdbarch))
> +	    return builtin_type_long_double;
> +	  else
> +	    return builtin_type_double;
> +	}
> +    }

Yes, Objective-C also.  Please clarify that "6.7" is a GDB version
number, or use a date.  Or just change it for all languages;
documenting "the value system uses C type promotions" is nice and
simple.  Either way is OK with me.

Beyond that the patch looked OK.

-- 
Daniel Jacobowitz
CodeSourcery