Fix overloading for 64-bit ints (revised)

The attached patch applied Rick Delaney's code to the
following math operations:

    neg (unary -)  abs  int  +  -  *  /  %  **

Tests have been added to lib/overload.t, and a new test
file
lib/overload64.t has been added (it is skipped if Perl is
not using 64-bit ints (i.e., $Config::Config{'uvsize'} !=
8)).

Rafael, I know you wanted to wait until after 5.10, but I
hope you'll reconsider.  I think this patch covers all the
requisite operations, the tests provided cover things well,
and all the tests in the test suite pass.

Also, after applying this patch, embed.pl needs to be run
to
regenerate some file, so that will also need to be done for
this
change.  Thanks.

(Patch was revised to fix a small compiler warning.)

On 18/10/2007, Jerry D. Hedden <jdheddencpan.org> wrote:
> The attached patch applied Rick Delaney's code to the
> following math operations:
>
>     neg (unary -)  abs  int  +  -  *  /  %  **
>
> Tests have been added to lib/overload.t, and a new test
file
> lib/overload64.t has been added (it is skipped if Perl
is
> not using 64-bit ints (i.e., $Config::Config{'uvsize'}
!= 8)).
>
> Rafael, I know you wanted to wait until after 5.10, but
I
> hope you'll reconsider.  I think this patch covers all
the
> requisite operations, the tests provided cover things
well,
> and all the tests in the test suite pass.

Thanks, applied as #32141.

On Oct 19 2007, Rafael Garcia-Suarez wrote:
> 
> Thanks, applied as #32141.

The attached patch adds the suggested macro to avoid the
extra function
call when the operand is not a reference.  No point
penalizing everyone
just for overload.

-- 
Rick Delaney
rickbort.ca

Rick Delaney wrote:
> The attached patch adds the suggested macro to avoid
the extra function
> call when the operand is not a reference.  No point
penalizing everyone
> just for overload.

Okay.  Should the SvROK() check in sv_2num be removed then
so that it's not done twice for overloaded operations?

--- perl-current/sv.c
+++ perl-current/sv.c
 -2513,9
+2513,6 
 SV *
 Perl_sv_2num(pTHX_ register SV *sv)
 {
-    if (!SvROK(sv))
-	return sv;
-
     if (SvAMAGIC(sv)) {
 	SV * const tmpsv = AMG_CALLun(sv,numer);
 	if (tmpsv && (!SvROK(tmpsv) || (SvRV(tmpsv) !=
SvRV(sv))))

(The above patch is also attached.)

Rick Delaney wrote:
> The attached patch adds the suggested macro to avoid
the extra function
> call when the operand is not a reference.  No point
penalizing everyone
> just for overload.

Jerry D. Hedden wrote:
> Okay.  Should the SvROK() check in sv_2num be removed
then
> so that it's not done twice for overloaded operations?
>
> --- perl-current/sv.c
> +++ perl-current/sv.c
>  -2513,9 +2513,6 
>  SV *
>  Perl_sv_2num(pTHX_ register SV *sv)
>  {
> -    if (!SvROK(sv))
> -       return sv;
> -
>      if (SvAMAGIC(sv)) {
>         SV * const tmpsv = AMG_CALLun(sv,numer);
>         if (tmpsv && (!SvROK(tmpsv) ||
(SvRV(tmpsv) != SvRV(sv))))

Revised patch attached that specifies using the SvNUM macro
in the apidocs.

On 20/10/2007, Jerry D. Hedden <jdheddencpan.org> wrote:
> > Okay.  Should the SvROK() check in sv_2num be
removed then
> > so that it's not done twice for overloaded
operations?
> >
> > --- perl-current/sv.c
> > +++ perl-current/sv.c
> >  -2513,9 +2513,6 
> >  SV *
> >  Perl_sv_2num(pTHX_ register SV *sv)
> >  {
> > -    if (!SvROK(sv))
> > -       return sv;
> > -
> >      if (SvAMAGIC(sv)) {
> >         SV * const tmpsv = AMG_CALLun(sv,numer);
> >         if (tmpsv && (!SvROK(tmpsv) ||
(SvRV(tmpsv) != SvRV(sv))))
>
> Revised patch attached that specifies using the SvNUM
macro in the apidocs.

Thanks, applied as #32148.

On 20/10/2007, Rafael Garcia-Suarez <rgarciasuarezgmail.com> wrote:
> Thanks, applied as #32148.
>

Well, no: I had to revert the code change, since it made
mkppport
enter an infinite loop. (I don't know why)

On Oct 19 2007, Jerry D. Hedden wrote:
> Rick Delaney wrote:
> > The attached patch adds the suggested macro to
avoid the extra function
> > call when the operand is not a reference.  No
point penalizing everyone
> > just for overload.
> 
> Okay.  Should the SvROK() check in sv_2num be removed
then
> so that it's not done twice for overloaded operations?

No, because the function should be able to cope with any SV.
 It's
better to do an extra flag check for the overload/reference
case which
are already slow/dubious than add an extra function call
for
non-references.  If we were going to worry about every extra
SvROK check
then we wouldn't even be able to write

    if (SvAMAGIC(sv))

but instead would want

    if (SvFLAGS(SvRV(sv)) & SVf_AMAGIC))

because SvAMAGIC first does an SvROK check.  There are
plenty of extra
Sv?OK checks in the source that we shouldn't prematurely
concern
ourselves with.

> --- perl-current/sv.c
> +++ perl-current/sv.c
>  -2513,9 +2513,6 
>  SV *
>  Perl_sv_2num(pTHX_ register SV *sv)
>  {
> -    if (!SvROK(sv))
> -	return sv;
> -
>      if (SvAMAGIC(sv)) {
>  	SV * const tmpsv = AMG_CALLun(sv,numer);
>  	if (tmpsv && (!SvROK(tmpsv) || (SvRV(tmpsv)
!= SvRV(sv))))
> 
> (The above patch is also attached.)



-- 
Rick Delaney
rickbort.ca

On Oct 20 2007, Rafael Garcia-Suarez wrote:
> On 20/10/2007, Rafael Garcia-Suarez
<rgarciasuarezgmail.com> wrote:
> > Thanks, applied as #32148.
> >
> 
> Well, no: I had to revert the code change, since it
made mkppport
> enter an infinite loop. (I don't know why)

Because you didn't apply the SvNUM macro part that the doc
patch says is
required to use sv_2num(). 

But I think the change to the function sv_2num() itself is
best reverted
anyway, whether the macro is added or not.

-- 
Rick Delaney
rickbort.ca

On 10/20/07, Rick Delaney <rickbort.ca> wrote:
> On Oct 20 2007, Rafael Garcia-Suarez wrote:
> > On 20/10/2007, Rafael Garcia-Suarez
<rgarciasuarezgmail.com> wrote:
> > > Thanks, applied as #32148.
> > >
> >
> > Well, no: I had to revert the code change, since
it made mkppport
> > enter an infinite loop. (I don't know why)
>
> Because you didn't apply the SvNUM macro part that the
doc patch says is
> required to use sv_2num(). 
>
> But I think the change to the function sv_2num() itself
is best reverted
> anyway, whether the macro is added or not.

Sorry that my little patches added as part of the
conversation confused the
issue.

Attached is a complete patch with Rick's work, and a
correction to the
API doc header.