Re: fix for expt bug

[Mark H Weaver]

Hi Ramakrishnan,

Thanks for the revised patch.  There are still some problems:

> diff --git a/libguile/numbers.c b/libguile/numbers.c
> index fbc6cc8..5bbf4b0 100644
> --- a/libguile/numbers.c
> +++ b/libguile/numbers.c
> @@ -5445,12 +5445,30 @@ SCM_DEFINE (scm_expt, "expt", 2, 0, 0,
>           "Return @var{x} raised to the power of @var{y}.") 
>  #define FUNC_NAME s_scm_expt
>  {
> -  if (scm_is_true (scm_exact_p (x)) && scm_is_integer (y))
> +  if (scm_is_true (scm_exact_p (x)) &&
> +      scm_is_true (scm_exact_p (y)) &&
> +      !SCM_FRACTIONP (y))
>      return scm_integer_expt (x, y);

This is an improvement, but isn't quite right.  scm_integer_expt
requires that y be an exact integer, but x is allowed to be any scheme
number whatsoever.  So the "scm_exact_p (x)" doesn't belong.  It should
simply be changed to "scm_exact_p (y)" instead.

The other problem is that !SCM_FRACTIONP is not the right test.
Although it is currently true that the only exact numbers in guile are
integers and rationals, there's no guarantee that other exact numbers
won't be added in the future.

The test above should be this:

    if (scm_is_true (scm_exact_p (y)) && scm_is_integer (y))

>    else if (scm_is_real (x) && scm_is_real (y) && scm_to_double (x) >= 0.0)
>      {
>        return scm_from_double (pow (scm_to_double (x), scm_to_double (y)));
>      }
> +  else if (scm_is_real (x) &&
> +           scm_is_real (y) &&
> +           (scm_to_double (x) < 0) &&
> +           !SCM_FRACTIONP (y))

The !SCM_FRACTIONP (y) does not belong here.  Instead, you should test
scm_is_integer (y), which will also make the scm_is_real (y) test
redundant.  As is, your code will produce the wrong answer if y is an
inexact floating-point number such as 0.5.  SCM_FRACTIONP returns true
only for exact rational numbers.

So the above test should be:

  else if (scm_is_real (x) && scm_is_integer (y) && scm_to_double (x) < 0)

> +    {
> +      SCM x_abs, result;
> +
> +      x_abs = scm_abs (x);

You've already established that x is a negative real, so it is wasteful
to do another test in scm_abs.  Just negate x.

Also, it's inefficient to create so many intermediate floating-point SCM
values, since it involves allocating heap cells.  You don't need x_abs,
and "result" can be a C double.  See my suggested code below.

> +      result = scm_from_double (pow (scm_to_double (x_abs), scm_to_double 
> (y)));
> +
> +      if (scm_is_true (scm_equal_p (y, scm_floor (y))) &&

The test above is better replaced by scm_is_integer (y), but it's needed
in the outer conditional anyway, so you can drop it here.

> +          scm_is_true (scm_odd_p (y)))
> +        return scm_product (scm_from_double (-1.0), result);
> +      else
> +        return result;
> +    }

In summary, I think the new clause should be something like: (UNTESTED!)

  else if (scm_is_real (x) && scm_is_integer (y) && scm_to_double (x) < 0)
  {
    double result;

    result = pow (-scm_to_double (x), scm_to_double (y));
    if (scm_is_true (scm_odd_p (y)))
      result = -result;
    return scm_from_double (result);
  }

     Best,
      Mark