From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Mark H Weaver Newsgroups: gmane.lisp.guile.devel Subject: Re: fix for expt bug Date: Tue, 02 Nov 2010 00:55:06 -0400 Message-ID: <87r5f4l3yt.fsf@yeeloong.netris.org> References: <8762whnc4d.fsf@yeeloong.netris.org> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: dough.gmane.org 1288673768 30291 80.91.229.12 (2 Nov 2010 04:56:08 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Tue, 2 Nov 2010 04:56:08 +0000 (UTC) Cc: guile-devel To: Ramakrishnan Muthukrishnan Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Tue Nov 02 05:56:03 2010 Return-path: Envelope-to: guile-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1PD8uF-0000Yb-BA for guile-devel@m.gmane.org; Tue, 02 Nov 2010 05:55:56 +0100 Original-Received: from localhost ([127.0.0.1]:45873 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PD8uC-0002MN-0T for guile-devel@m.gmane.org; Tue, 02 Nov 2010 00:55:32 -0400 Original-Received: from [140.186.70.92] (port=47792 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PD8u6-0002MD-QH for guile-devel@gnu.org; Tue, 02 Nov 2010 00:55:28 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PD8u5-00070W-BQ for guile-devel@gnu.org; Tue, 02 Nov 2010 00:55:26 -0400 Original-Received: from world.peace.net ([216.204.32.208]:35334) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PD8u5-0006zn-6R for guile-devel@gnu.org; Tue, 02 Nov 2010 00:55:25 -0400 Original-Received: from ip68-9-118-38.ri.ri.cox.net ([68.9.118.38] helo=freedomincluded) by world.peace.net with esmtpa (Exim 4.69) (envelope-from ) id 1PD8to-0002U2-TR; Tue, 02 Nov 2010 00:55:09 -0400 Original-Received: from mhw by freedomincluded with local (Exim 4.69) (envelope-from ) id 1PD8tn-0003tR-2a; Tue, 02 Nov 2010 00:55:07 -0400 In-Reply-To: (Ramakrishnan Muthukrishnan's message of "Mon, 1 Nov 2010 23:53:46 +0530") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-BeenThere: guile-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Developers list for Guile, the GNU extensibility library" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Errors-To: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.lisp.guile.devel:11098 Archived-At: 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