Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.94k stars 554 forks source link

bignum >= 0.51 breaks BigInt >> #21370

Open daniel-pfeiffer opened 1 year ago

daniel-pfeiffer commented 1 year ago

This is a bug report for perl from occitan@esperanto.org, generated with the help of perlbug 1.42 running under perl 5.36.0.


Devuan Daedalus (testing, based on Debian Bookworm 12), comes with two installed Perl versions. Both break Math::BigInt badly, turning an integer right shift one into a floating division by two. While they both seem to be using their Perl's respective version of bignum, they seem to have a bleeding edge Math::BigInt, slightly ahead even of Perl 5.38:

Perl 5.32.1 bignum 0.51

$ /usr/bin/perl5.32-x86_64-linux-gnu -Mbignum -e 'use 5.32.1;
    say for $bignum::VERSION, $Math::BigInt::VERSION, Math::BigInt->new(3) >> 1, 3 >> 1'
0.51
1.999838
1.5
1.5

$ /usr/bin/perl5.32-x86_64-linux-gnu -M'bignum upgrade => undef' -e 'use 5.32.1;
    say for Math::BigInt->new(3) >> 1, 3 >> 1'
1
1

$ /usr/bin/perl5.32-x86_64-linux-gnu -MMath::BigInt -e 'use 5.32.1;
    say for $Math::BigInt::VERSION, Math::BigInt->new(3) >> 1'
1.999838
1

Perl 5.36.0 bignum 0.65

$ perl -Mbignum -e 'use 5.36.0;
    say for $bignum::VERSION, $Math::BigInt::VERSION, Math::BigInt->new(3) >> 1, 3 >> 1'
0.65
1.999838
1.5
1.5

$ perl -M'bignum upgrade => undef' -e 'use 5.36.0;
    say for Math::BigInt->new(3) >> 1, 3 >> 1'
1
1

$ perl -MMath::BigInt -e 'use 5.36.0;
    say for $Math::BigInt::VERSION, Math::BigInt->new(3) >> 1'
1.999838
1


Flags: category=library severity=medium module=bignum

Site configuration information for perl 5.36.0:

Configured by Debian at Sun Jan 8 21:28:47 UTC 2023.

Summary of my perl5 (revision 5 version 36 subversion 0) configuration:

Platform: osname=linux osvers=4.19.0 archname=x86_64-linux-gnu-thread-multi uname='linux localhost 4.19.0 #1 smp debian 4.19.0 x86_64 gnulinux ' config_args='-Dmksymlinks -Dusethreads -Duselargefiles -Dcc=x86_64-linux-gnu-gcc -Dcpp=x86_64-linux-gnu-cpp -Dld=x86_64-linux-gnu-gcc -Dccflags=-DDEBIAN -Wdate-time -D_FORTIFY_SOURCE=2 -g -O2 -ffile-prefix-map=/dummy/build/dir=. -fstack-protector-strong -Wformat -Werror=format-security -Dldflags= -Wl,-z,relro -Dlddlflags=-shared -Wl,-z,relro -Dcccdlflags=-fPIC -Darchname=x86_64-linux-gnu -Dprefix=/usr -Dprivlib=/usr/share/perl/5.36 -Darchlib=/usr/lib/x86_64-linux-gnu/perl/5.36 -Dvendorprefix=/usr -Dvendorlib=/usr/share/perl5 -Dvendorarch=/usr/lib/x86_64-linux-gnu/perl5/5.36 -Dsiteprefix=/usr/local -Dsitelib=/usr/local/share/perl/5.36.0 -Dsitearch=/usr/local/lib/x86_64-linux-gnu/perl/5.36.0 -Dman1dir=/usr/share/man/man1 -Dman3dir=/usr/share/man/man3 -Dsiteman1dir=/usr/local/man/man1 -Dsiteman3dir=/usr/local/man/man3 -Duse64bitint -Dman1ext=1 -Dman3ext=3perl -Dpager=/usr/bin/sensible-pager -Uafs -Ud_csh -Ud_ualarm -Uusesfio -Uusenm -Ui_libutil -Ui_xlocale -Uversiononly -DDEBUGGING=-g -Doptimize=-O2 -dEs -Duseshrplib -Dlibperl=libperl.so.5.36.0' hint=recommended useposix=true d_sigaction=define useithreads=define usemultiplicity=define use64bitint=define use64bitall=define uselongdouble=undef usemymalloc=n default_inc_excludes_dot=define Compiler: cc='x86_64-linux-gnu-gcc' ccflags ='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fwrapv -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64' optimize='-O2 -g' cppflags='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fwrapv -fno-strict-aliasing -pipe -I/usr/local/include' ccversion='' gccversion='12.2.0' gccosandvers='' intsize=4 longsize=8 ptrsize=8 doublesize=8 byteorder=12345678 doublekind=3 d_longlong=define longlongsize=8 d_longdbl=define longdblsize=16 longdblkind=3 ivtype='long' ivsize=8 nvtype='double' nvsize=8 Off_t='off_t' lseeksize=8 alignbytes=8 prototype=define Linker and Libraries: ld='x86_64-linux-gnu-gcc' ldflags =' -fstack-protector-strong -L/usr/local/lib' libpth=/usr/local/lib /usr/lib/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib libs=-lgdbm -lgdbm_compat -ldb -ldl -lm -lpthread -lc -lcrypt perllibs=-ldl -lm -lpthread -lc -lcrypt libc=/lib/x86_64-linux-gnu/libc.so.6 so=so useshrplib=true libperl=libperl.so.5.36 gnulibc_version='2.36' Dynamic Linking: dlsrc=dl_dlopen.xs dlext=so d_dlsymun=undef ccdlflags='-Wl,-E' cccdlflags='-fPIC' lddlflags='-shared -L/usr/local/lib -fstack-protector-strong'

Locally applied patches: DEBPKG:debian/cpan_definstalldirs - Provide a sensible INSTALLDIRS default for modules installed from CPAN. DEBPKG:debian/db_file_ver - https://bugs.debian.org/340047 Remove overly restrictive DB_File version check. DEBPKG:debian/doc_info - Replace generic man(1) instructions with Debian-specific information. DEBPKG:debian/enc2xs_inc - https://bugs.debian.org/290336 Tweak enc2xs to follow symlinks and ignore missing @INC directories. DEBPKG:debian/errno_ver - https://bugs.debian.org/343351 Remove Errno version check due to upgrade problems with long-running processes. DEBPKG:debian/libperl_embed_doc - https://bugs.debian.org/186778 Note that libperl-dev package is required for embedded linking DEBPKG:fixes/respect_umask - Respect umask during installation DEBPKG:debian/writable_site_dirs - Set umask approproately for site install directories DEBPKG:debian/extutils_set_libperl_path - EU:MM: set location of libperl.a under /usr/lib DEBPKG:debian/no_packlist_perllocal - Don't install .packlist or perllocal.pod for perl or vendor DEBPKG:debian/fakeroot - Postpone LD_LIBRARY_PATH evaluation to the binary targets. DEBPKG:debian/instmodsh_doc - Debian policy doesn't install .packlist files for core or vendor. DEBPKG:debian/ld_run_path - Remove standard libs from LD_RUN_PATH as per Debian policy. DEBPKG:debian/libnet_config_path - Set location of libnet.cfg to /etc/perl/Net as /usr may not be writable. DEBPKG:debian/perlivp - https://bugs.debian.org/510895 Make perlivp skip include directories in /usr/local DEBPKG:debian/squelch-locale-warnings - https://bugs.debian.org/508764 Squelch locale warnings in Debian package maintainer scripts DEBPKG:debian/patchlevel - https://bugs.debian.org/567489 List packaged patches for 5.36.0-7 in patchlevel.h DEBPKG:fixes/document_makemaker_ccflags - https://bugs.debian.org/628522 [rt.cpan.org #68613] Document that CCFLAGS should include $Config{ccflags} DEBPKG:debian/find_html2text - https://bugs.debian.org/640479 Configure CPAN::Distribution with correct name of html2text DEBPKG:debian/perl5db-x-terminal-emulator.patch - https://bugs.debian.org/668490 Invoke x-terminal-emulator rather than xterm in perl5db.pl DEBPKG:debian/cpan-missing-site-dirs - https://bugs.debian.org/688842 Fix CPAN::FirstTime defaults with nonexisting site dirs if a parent is writable DEBPKG:fixes/memoize_storable_nstore - [rt.cpan.org #77790] https://bugs.debian.org/587650 Memoize::Storable: respect 'nstore' option not respected DEBPKG:debian/makemaker-pasthru - https://bugs.debian.org/758471 Pass LD settings through to subdirectories DEBPKG:debian/makemaker-manext - https://bugs.debian.org/247370 Make EU::MakeMaker honour MANnEXT settings in generated manpage headers DEBPKG:debian/kfreebsd-softupdates - https://bugs.debian.org/796798 Work around Debian Bug#796798 DEBPKG:fixes/memoize-pod - [rt.cpan.org #89441] Fix POD errors in Memoize DEBPKG:debian/hurd-softupdates - https://bugs.debian.org/822735 Fix t/op/stat.t failures on hurd DEBPKG:fixes/math_complex_doc_great_circle - https://bugs.debian.org/697567 [rt.cpan.org #114104] Math::Trig: clarify definition of great_circle_midpoint DEBPKG:fixes/math_complex_doc_see_also - https://bugs.debian.org/697568 [rt.cpan.org #114105] Math::Trig: add missing SEE ALSO DEBPKG:fixes/math_complex_doc_angle_units - https://bugs.debian.org/731505 [rt.cpan.org #114106] Math::Trig: document angle units DEBPKG:fixes/cpan_web_link - https://bugs.debian.org/367291 CPAN: Add link to main CPAN web site DEBPKG:debian/hppa_op_optimize_workaround - https://bugs.debian.org/838613 Temporarily lower the optimization of op.c on hppa due to gcc-6 problems DEBPKG:debian/installman-utf8 - https://bugs.debian.org/840211 Generate man pages with UTF-8 characters DEBPKG:debian/hppa_opmini_optimize_workaround - https://bugs.debian.org/869122 Lower the optimization level of opmini.c on hppa DEBPKG:debian/sh4_op_optimize_workaround - https://bugs.debian.org/869373 Also lower the optimization level of op.c and opmini.c on sh4 DEBPKG:debian/perldoc-pager - https://bugs.debian.org/870340 [rt.cpan.org #120229] Fix perldoc terminal escapes when sensible-pager is less DEBPKG:debian/prune_libs - https://bugs.debian.org/128355 Prune the list of libraries wanted to what we actually need. DEBPKG:debian/mod_paths - Tweak @INC ordering for Debian DEBPKG:debian/deprecate-with-apt - https://bugs.debian.org/747628 Point users to Debian packages of deprecated core modules DEBPKG:debian/disable-stack-check - https://bugs.debian.org/902779 [GH #16607] Disable debugperl stack extension checks for binary compatibility with perl DEBPKG:debian/perlbug-editor - https://bugs.debian.org/922609 Use "editor" as the default perlbug editor, as per Debian policy DEBPKG:debian/eu-mm-perl-base - https://bugs.debian.org/962138 Suppress an ExtUtils::MakeMaker warning about our non-default @INC DEBPKG:fixes/io_socket_ip_ipv6 - Disable getaddrinfo(3) AI_ADDRCONFIG for localhost and IPv4 numeric addresses DEBPKG:debian/usrmerge-lib64 - https://bugs.debian.org/914128 Configure / libpth.U: Do not adjust glibpth when /usr/lib64 is present. DEBPKG:debian/usrmerge-realpath - https://bugs.debian.org/914128 Configure / libpth.U: use realpath --no-symlinks on Debian DEBPKG:debian/configure-regen - https://bugs.debian.org/762638 Regenerate Configure et al. after probe unit changes DEBPKG:fixes/x32-io-msg-skip - https://bugs.debian.org/922609 Skip io/msg.t on x32 due to broken System V message queues DEBPKG:debian/hurd-eumm-workaround - https://bugs.debian.org/1018289 Work around a MakeMaker regression breaking GNU/Hurd hint files DEBPKG:fixes/json-pp-warnings - https://bugs.debian.org/1019757 Call unimport first to silence warnings DEBPKG:fixes/readline-stream-errors - [80c1f1e] [GH #6799] https://bugs.debian.org/1016369 only clear the stream error state in readline() for glob() DEBPKG:fixes/readline-stream-errors-test - [0b60216] [GH #6799] https://bugs.debian.org/1016369 test that <> doesn't clear the stream error state DEBPKG:fixes/lto-test-fix - [69b4fa3] [GH #20518] https://bugs.debian.org/1015579 skip checking categorization of libperl symbols for LTO builds


@INC for perl 5.36.0: /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.36.0 /usr/local/share/perl/5.36.0 /usr/lib/x86_64-linux-gnu/perl5/5.36 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl-base /usr/lib/x86_64-linux-gnu/perl/5.36 /usr/share/perl/5.36 /usr/local/lib/site_perl


Environment for perl 5.36.0: HOME=/home/pfeiffer LANG=eo.utf8 LANGUAGE (unset) LC_ALL=eo.utf8 LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/home/pfeiffer/.cargo/bin:/home/pfeiffer/bin:/home/software/eclipse:/usr/local/sbin:/home/software/jdk/bin:/usr/sbin:/sbin:/usr/local/bin:/usr/bin:/bin PERL_BADLANG (unset) SHELL=/bin/bash

hvds commented 1 year ago

This sounds like it may be connected to earlier ticket #19539; see also related danaj/Math-Prime-Util#61 and bignum#141772.

The comment from Math::BigInt author @pjacklam here may be of particular relevance.

jkeenan commented 1 year ago

Consider these two programs:

$ cat gh-21370-BigFloat.pl 
#!/usr/bin/env perl
use 5.14.0;
use warnings;

#use bignum;
use Math::BigInt;

say "perl VERSION:\t\t", $^V;
#say "bignum VERSION:\t\t", $bignum::VERSION;
say "Math::BigInt VERSION:\t", $Math::BigInt::VERSION;
say '';
say Math::BigInt->new(3) >> 1, 3 >> 1;
say Math::BigInt->new(3) >> 1, '|', 3 >> 1;

and

$ cat gh-21370-bignum.pl
#!/usr/bin/env perl
use 5.14.0;
use warnings;

use bignum;
use Math::BigInt;

say "perl VERSION:\t\t", $^V;
say "bignum VERSION:\t\t", $bignum::VERSION;
say "Math::BigInt VERSION:\t", $Math::BigInt::VERSION;
say '';
say Math::BigInt->new(3) >> 1, 3 >> 1;
say Math::BigInt->new(3) >> 1, '|', 3 >> 1;

... which differ only in that the second has use bignum. Now let's run them in the two most recent production releases of perl.

perl-5.36.0

$ perlbrew use perl-5.36.0
$ perl gh-21370-BigFloat.pl 
perl VERSION:       v5.36.0
Math::BigInt VERSION:   1.999830

11
1|1

$ perl gh-21370-bignum.pl
perl VERSION:       v5.36.0
bignum VERSION:     0.65
Math::BigInt VERSION:   1.999830

11
1|1

perl-5.38.0

$ perlbrew use perl-5.38.0
$ perl gh-21370-BigFloat.pl 
perl VERSION:       v5.38.0
Math::BigInt VERSION:   1.999837

11
1|1

$ perl gh-21370-bignum.pl
perl VERSION:       v5.38.0
bignum VERSION:     0.66
Math::BigInt VERSION:   1.999837

1.51.5
1.5|1.5

@daniel-pfeiffer, does this accurately represent the problem you are experiencing?

pjacklam commented 1 year ago

I have looked at this, and unfortunately it is not clear to me what the problem is. I would be grateful for an example showing what is expected vs. how it is working.

When I took over maintenance, the bignum pragma, as well as the upgrading/downgrading functionality in Math::BigInt and Math::BigFloat, were poorly documented and had an inconsistent behaviour. I was uncertain about how it was supposed to be working. Unfortunately, there was some back and forth until I was able to make it work as I believe it was intended by the original author. Anyway, here is how it works (or is supposed to work) now:

The bignum pragma converts every numerical literal into a Math::BigInt or Math::BigFloat, depending on whether the literal represents an integer (or Inf or NaN) or non-integer, respectively.

The bignum pragma enables upgrading and downgrading, so any function or operand giving a non-integer result, returns a Math::BigFloat. This applies also when the operands are Math::BigInt objects. An example of this is

$ perl -Mbignum -wle 'print 3 / 2'
1.5

And vice versa: Any function or operand giving an integer (or Inf or NaN) returns a Math::BigInt. This applies also when the operands are Math::BigFloat objects. An example of this is

$ perl -Mbignum -wle 'print 1.5 + 1.5'
3

Earlier versions of bignum/Math::BigInt/Math::BigFloat were inconsistent: Some functions and operators would upgrade whereas others wouldn't, even in equivalent cases. Some functions would upgrade only in certain cases, but not in other, similar cases. Ditto for downgrading. The whole thing seemed rather arbitrary. At least it should be consistent now.

sisyphus commented 1 year ago

The bignum pragma enables upgrading and downgrading, so any function or operand giving a non-integer result, returns a Math::BigFloat. This applies also when the operands are Math::BigInt objects. An example of this is $ perl -Mbignum -wle 'print 3 / 2' 1.5

There are probably sound arguments that support that behaviour, but I think @daniel-pfeiffer is complaining about the following behaviour:

>perl -Mbignum -le "print 7 >> 1;"
3.5

"7" is an integer, and so is 7 >> 1, so there's a reasonable expectation (IMO) that the result should be (BigInt) 3, not (BigFloat) 3.5.

EDIT: I'm running bignum-0.66 and Math-BigInt-1.999839 on perl-5.38.0.

Cheers, Rob

pjacklam commented 1 year ago

There are probably sound arguments that support that behaviour, but I think @daniel-pfeiffer is complaining about the following behaviour:

>perl -Mbignum -le "print 7 >> 1;"
3.5

"7" is an integer, and so is 7 >> 1, so there's a reasonable expectation (IMO) that the result should be (BigInt) 3, not (BigFloat) 3.5.

Ah, I see. The simple solution is to use bigint:

$ perl -Mbigint -le "print 7 >> 1;" 3

When using bignum, 7 >> 1 only returns 3.5 because bignum upgrades the operands from BigInts to BigFloats. So I guess the essential questions here are:

Should >> upgrade? Is >> an operator that is inherently limited to integers (i.e., like the binary |, &, ^, and unary ~)?

The >> operator simply calls brsft(). When brsft() was discussed some years ago, my impression was that the majority wanted brsft() to do integer right shift regardless of the class of the operands. So I modified brsft() accordingly. At that point 7 >> 1 returned 3, even when the operands were BigFloats. As a result, other people were furious, arguing that brsft() is essentially just a division by a power of 2, which is not at all limited to integers. So I changed it back. These changes to brsft() directly affected >>.

I see good arguments both for letting >> always treat its operands as integers, and for letting >> be a "true" division.

EDIT: I'm running bignum-0.66 and Math-BigInt-1.999839 on perl-5.38.0.

From (and including) Math-BigInt-1.999832, 7 >> 1 gives 3.5 with all version of bignum back to (and including) the ancient bignum-0.05.

daniel-pfeiffer commented 1 year ago

I stumbled across this when regenerating the pl website. For the examples with a ▶ play button (that pops up the output) I mostly just pluck the code from .pod and run it. Last time I generated the page months ago (not sure which bignum/BigInt/perl version Devuan had back then) this worked.

This weekend the 3rd example for Collatz was suddenly failing – with to_base refusing to format a float with a riddle message “the value to convert must be a finite, non-negative integer.” So I explored and found I can turn off upgrading (hours wasted…)

Now I find shifting floats an amusing edge case, I had never even considered. But shifting ints should never ever upgrade! That totally breaks the semantic of shifting, with bignum taking a nanny attitude of “what you really want is division.” Well no thank you, I have a / on my keyboard, if that were what I really want.

I wasn't aware of bigint, sounds like a good workaround. ~It doesn't document to_base – anybody aware which minimum Perl version introduced that? (I have 5.18.2 without, and 5.28.1with – so it must have been introduced between these.)~ Edit: it seems that Devuan has muddled include pathes, partially specific to the Perl version, but Math is shared from /usr/share/perl5, so it doesn't reflect upstream versions.

sisyphus commented 1 year ago

Ah, I see. The simple solution is to use bigint

I was thinking a better solution would be to have the overloaded >> operator perform an actual right shift if ref($num) eq 'Math::BigInt' and do a division only if ref($num) eq 'Math::BigFloat'. That way you can still use bignum && have the >> operator DWIM for both BigInts and BigFloats. (Of course, I have no idea how difficult that might be to implement, though it does sound pretty easy ;-)

But then .... maybe that's not such a good idea. As $num jumps back and forth between BigInt and BigFloat (as can happen) people will probably go nuts trying to keep track of whether the >> operators are doing right shifts or divide-by-twos.

Cheers, Rob

pjacklam commented 1 year ago

I was thinking a better solution would be to have the overloaded >> operator perform an actual right shift if ref($num) eq 'Math::BigInt' and do a division only if ref($num) eq 'Math::BigFloat'. That way you can still use bignum && have the >> operator DWIM for both BigInts and BigFloats.

I fear that will be confusing. The Perl documentation (perldoc perlop) says

Binary ">>" returns the value of its left argument shifted right by the number of bits specified by the right argument. Arguments should be integers. (See also "Integer Arithmetic".)

so I think the best solution is to let << and >> do bitwise left and right shift, respectively, regardless of the class of the operands. An operand that is a Math::BigFloat or Math::BigRat will be truncated to an integer before the right/left shift is performed. This is consistent with the Perl documentation and the idea behind bignum which is, I believe, to add transparent support for arbitrarily large numbers.

The methods blsft() and brsft() should remain as they are and essentially be shorthands for multiplication and division, respectively. So $x -> brsft($y, $b) will remain equivalent to $x -> bdiv($b -> bpow($y)) and $x -> blsft($y, $b) will remain equivalent to $x -> bmul($b -> bpow($y)). Both blsft() and brsft() will upgrade and downgrade as necessary.

Currently, << is implemented by calling blsft() directly, and >> is implemented by calling brsft() directly, but his has to change.

Does this sound like a good idea?

sisyphus commented 1 year ago

On Sat, Aug 19, 2023 at 9:00 PM Peter John Acklam @.***> wrote:

I was thinking a better solution would be to have the overloaded >> operator perform an actual right shift if ref($num) eq 'Math::BigInt' and do a division only if ref($num) eq 'Math::BigFloat'. That way you can still use bignum && have the >> operator DWIM for both BigInts and BigFloats.

I fear that will be confusing. The Perl documentation (perldoc perlop) says

Binary ">>" returns the value of its left argument shifted right by the number of bits specified by the right argument. Arguments should be integers. (See also "Integer Arithmetic".)

so I think the best solution is to let << and >> do bitwise left and right shift, respectively, regardless of the class of the operands. An operand that is a Math::BigFloat or Math::BigRat will be truncated to an integer before the right/left shift is performed. This is consistent with the Perl documentation and the idea behind bignum which is, I believe, to add transparent support for arbitrarily large numbers.

IIUC, this would mean that, under bignum the condition $n >> $s == int($n)

$s is always true. That sounds fine to me Was that something you had tried to implement previously, only for it to be met with resistance ?

The methods blsft() and brsft() should remain as they are and essentially

be shorthands for multiplication and division, respectively. So $x -> brsft($y, $b) will remain equivalent to $x -> bdiv($b -> bpow($y)) and $x -> blsft($y, $b) will remain equivalent to $x -> bmul($b -> bpow($y)). Both blsft() and brsft() will upgrade and downgrade as necessary.

Currently, << is implemented by calling blsft() directly, and >> is implemented by calling brsft() directly, but his has to change.

I probably haven't quite got my head around that. For a Math::BigInt $x: 1) will $x >> $s still be equivalent to $x->brsft($s) ? && 2) will $x << $s still be equivalent to $x->blsft($s) ?

If so, then that's fine, too.

I'm not too concerned about how Math::BigFloats/Math::BigRats are treated by the shift operators. They're not proper ops to use with floats/rats... so therefore you can implement them to do whatever you want, and people are free to use them if they behave as desired, or free to not use them if they find the behaviour to not be as desired. IMO. there's really no grounds for complaint about the shift implementations you choose to provide for Math::BigFloats and Math::BigRats

Does this sound like a good idea?

Cheers, Rob

pjacklam commented 1 year ago

I think the best solution is to let << and >> do bitwise left and right shift, respectively, regardless of the class of the operands. An operand that is a Math::BigFloat or Math::BigRat will be truncated to an integer before the right/left shift is performed. This is consistent with the Perl documentation and the idea behind bignum which is, I believe, to add transparent support for big numbers.

IIUC, this would mean that, under bignum the condition $n >> $s == int($n) >> $s is always true. That sounds fine to me.

Actually, since both operands might be non-integers, it is the condition $n >> $s == int($n) >> int($s) that will always be true. This is just like core Perl, where bitwise operators truncate their operands to integers.

Was that something you had tried to implement previously, only for it to be met with resistance ?

I wanted to create the functionality we are discussing here, i.e., that >> truncates its operands to integers, just like core Perl. Now, since >> is implemented simply by calling brsft(), what I did was to modify brsft() so it gave the functionality I wanted. This was a mistake. What I should have done, was to create a new subroutine (or method) for >> and let brsft() be untouched.

The methods blsft() and brsft() should remain as they are and essentially be shorthands for multiplication and division, respectively. So $x -> brsft($y, $b) will remain equivalent to $x -> bdiv($b -> bpow($y)) and $x -> blsft($y, $b) will remain equivalent to $x -> bmul($b -> bpow($y)). Both blsft() and brsft() will upgrade and downgrade as necessary. Currently, << is implemented by calling blsft() directly, and >> is implemented by calling brsft() directly, but his has to change.

I probably haven't quite got my head around that. For a Math::BigInt $x: 1) will $x >> $s still be equivalent to $x->brsft($s) ? && 2) will $x << $s still be equivalent to $x->blsft($s) ? `

My idea is to let >> and << be bitwise shift operators. They will truncate any non-integer operand to integer and never upgrade. This is how core Perl does it, so it will be in accordance with the idea that bignum introduces transparent support for big numbers.

So when bigint is in effect, $x >> $s will be equivalent to $x -> brsft($s) and $x << $s will be equivalent to $x -> blsft($s). However, when bignum is in effect, brsft() and blsft() might upgrade, as they do now, but << and >> will remain unchanged and behave like << and >> in core Perl: They will truncate their operands to integers and never upgrade.

sisyphus commented 1 year ago

This is how core Perl does it, so it will be in accordance with the idea that bignum introduces transparent support for big numbers.

Yep - I can't think of anything better than following perl's lead on this.

I might even change Math::MPFR's overloading of << and >> to do the same. (I doubt anyone will notice ;-) Could you bump this thread when these changes have been implemented and made available for testing and/or installation ? That way I shouldn't miss it.

Cheers, Rob

pjacklam commented 1 year ago

Could you bump this thread when these changes have been implemented and made available for testing and/or installation? That way I shouldn't miss it.

If I remember, I will post it here, when I have something working.

While I am at it, I will also fix some very old bugs in blsft() and brsft(). For example, the following should return Inf, not NaN:

$ perl -MMath::BigInt -wle 'print Math::BigInt -> blsft(1, "Inf")'
NaN

and the following should return 0, not NaN:

$ perl -MMath::BigInt -wle 'print Math::BigInt -> brsft(1, "Inf")'
NaN
hvds commented 1 year ago

Was that something you had tried to implement previously, only for it to be met with resistance ?

I wanted to create the functionality we are discussing here, i.e., that >> truncates its operands to integers, just like core Perl. Now, since >> is implemented simply by calling brsft(), what I did was to modify brsft() so it gave the functionality I wanted. This was a mistake. What I should have done, was to create a new subroutine (or method) for >> and let brsft() be untouched.

Ah, I hadn't understood this detail of the history. If the pushback was indeed primarily because of changes to brsft() rather than changes to >>, then your proposed solution sounds ideal to me.