Re: [RFA] BINOP_DIV and ptyp command

Daniel Jacobowitz
2008-01-31T22:48:25Z

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