RE: [RFA] Handle BINOP_INTDIV in valarith.c

["Pierre Muller" <muller@ics.u-strasbg.fr>]

OK, I get the idea,
but the problem is that this leads to
lots of code copy...
And errors are duplicated at several places:
  See for instance my bug report:
http://sourceware.org/ml/gdb/2008-01/msg00209.html

'ptyp 3 / 2.0'
which returns 'int' type
while 'print 3 / 2.0' returns '1.5' which is not an int!

I suspect that the code in Ada shows the same bugs:

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Joel Brobecker
> Sent: Thursday, January 24, 2008 12:09 AM
> To: Pierre Muller
> Cc: 'Eli Zaretskii'; gdb-patches@sourceware.org
> Subject: Re: [RFA] Handle BINOP_INTDIV in valarith.c
> 
> >   I wrote the floatdiv version, which allows me to easily force the
> >   conversion to double formats for left and right operand:
> 
> Right, but you are doing the conversion inside what I call "core-gdb",
> which is a language-independent file. As a result, you ended up needing
> a new operator.  But the creation of this new operator would not be
> necessary if you handled BINOP_DIV in a pascal-specific way.
> 
> >    maybe
> >    || (op == BINOP_DIV && current_language = language_pascal))
> 
> That's not the proper way of doing this. What you need to do is to
> setup your language to have a pascal-specific expression evaluator.
> Right now, it's set to use the "standard" expression evaluator which
> (IIRC) is the C evaluator.  Your pascal expression evaluator will
> defer back to the standard expression evaluator most of the time,
> except for the case when the binary operator is FLOAT_DIV.  In this
> case, you'll do the division yourself.
> 
> You can have a look at ada-lang.c:ada_evaluate_subexp(). It's a large
> routine because of a lot of stuff is special to Ada, but you'll also
> see:
> 
>   op = exp->elts[pc].opcode;
>   [...]
>   switch (op)
>     {
>     default:
>       *pos -= 1;
>       arg1 = evaluate_subexp_standard (expect_type, exp, pos, noside);
>     [...]
>     case BINOP_DIV:
>       arg1 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
>       arg2 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
>       if (noside == EVAL_SKIP)
>         goto nosideret;
>       else if (noside == EVAL_AVOID_SIDE_EFFECTS
>                && (op == BINOP_DIV || op == BINOP_REM || op ==
> BINOP_MOD))
>         return value_zero (value_type (arg1), not_lval);
which means that 'ptyp' which sets noside to EVAL_AVOID_SIDE_EFFECTS
will not see the fixed to double conversions below
and will also return 'fixed', while the result of print will be
a 'double'.
  Furthermore, you can clearly see that this code
was copied from the C evaluator without removing the 
unnecessary parts:
the check
>                && (op == BINOP_DIV || op == BINOP_REM || op ==
> BINOP_MOD))
is clearly redundant here as we are inside case BINOP_DIV.

The same unnecessary test is also done in the
BINOP_MOD-BINOP_REM case.

>       else
>         {
>           if (ada_is_fixed_point_type (value_type (arg1)))
>             arg1 = cast_from_fixed_to_double (arg1);
>           if (ada_is_fixed_point_type (value_type (arg2)))
>             arg2 = cast_from_fixed_to_double (arg2);
>           return ada_value_binop (arg1, arg2, op);
>         }
> 
> If you look at the "default:" case, you'll see that we let
> the standard evaluator do the work for us. But we treat the
> BINOP_DIV case specially: First we evaluate each side of our
> division, and then perform the division and return the result.
> 
> This ada_evaluate_subexp routine is setup in the language through
> the exp_descriptor structure.

  I agree with you that writing a pascal specific
evaluate_subexp() function is better that
explicitly checking the language of the currently parsed expression,
but my idea was really to have on the contrary
something usable for other languages, and thus not inside 
pascal specific code...
  
  On the other hand, no one said that 
another language has the same view on the '/' operator as pascal...
Whihch validates your view that a new BINOP is not necessary...
So I will probably try to write a pascal_evaluate_subexp function
as you suggested.

  Thanks for the feedback.

Pierre Muller