Bug #120288 for Math-GMP: $x += $gmp warns on $x undef

Sat Feb 18 09:03:40 2017 HVDS [...] cpan.org - Ticket created

Subject:

$x += $gmp warns on $x undef

% perl -MMath::GMP -we 'my $x; $x += 1' % perl -MMath::GMP -we 'my $x; $x += Math::GMP->new(1)' Use of uninitialized value in null operation at -e line 1. % I think this should not warn (and even if it should, I'm sure it should not be "in null operation"). I plan to investigate why that's happening, but if you happen to know feel free to jump in. :) Hugo

Tue Feb 21 08:25:52 2017 HVDS [...] cpan.org - Correspondence added

On Sat Feb 18 09:03:40 2017, HVDS wrote: Show quoted text

> % perl -MMath::GMP -we 'my $x; $x += 1' > % perl -MMath::GMP -we 'my $x; $x += Math::GMP->new(1)' > Use of uninitialized value in null operation at -e line 1. > % > > I think this should not warn (and even if it should, I'm sure it > should not be "in null operation"). I plan to investigate why that's > happening, but if you happen to know feel free to jump in. :)

This happens during the declaration of the arguments to op_add: #0 Perl_report_uninit (uninit_sv=0xa91748) at sv.c:16608 #1 0x00000000005d45f8 in Perl_sv_2pv_flags (sv=0xa91748, lp=0x0, flags=2) at sv.c:3196 #2 0x00007ffff6ea7766 in sv2gmp (sv=0xa91748) at GMP.xs:106 #3 0x00007ffff6ea9aa8 in XS_Math__GMP_op_add (cv=0xabea70) at GMP.c:674 #4 0x00000000005bf736 in Perl_pp_entersub () at pp_hot.c:4231 #5 0x000000000048b6a9 in Perl_amagic_call (left=0xa91748, right=0xa74190, method=30, flags=4) at gv.c:3494 #6 0x0000000000485e16 in Perl_try_amagic_bin (method=30, flags=20) at gv.c:2976 #7 0x00000000005a9834 in Perl_pp_add () at pp_hot.c:601 [...] .. where #3 corresponds to the declaration "mpz_t * n", which expands to: mpz_t * n = sv2gmp(ST(1)); and then hits this from sv2gmp: pv = SvPV_nolen(sv); I don't see an easy remedy for this without adding a lot of complexity (by declaring the arguments for a bunch of ops as SV* rather than mpz_t*, and then explicitly handling the calls to sv2gmp with extra invocation-specific checks), which seems hard to justify for the benefit. I'll leave it open in case someone else can see a simpler route. Hugo

Sun Feb 26 05:48:37 2017 SHLOMIF [...] cpan.org - Correspondence added

Hi Hugo, On Sat Feb 18 09:03:40 2017, HVDS wrote: Show quoted text

> % perl -MMath::GMP -we 'my $x; $x += 1' > % perl -MMath::GMP -we 'my $x; $x += Math::GMP->new(1)' > Use of uninitialized value in null operation at -e line 1. > % >

The more idiomatic and recommended way would be to do "my $x = 0; $x += ...", so the warning may actually be a good thing. I'm marking this ticket as "stalled" because I don't think it's worthy of a fix, due to the fact that the user is doing something wrong. Show quoted text

> I think this should not warn (and even if it should, I'm sure it > should not be "in null operation"). I plan to investigate why that's > happening, but if you happen to know feel free to jump in. :) > > Hugo

Sun Feb 26 05:48:37 2017 The RT System itself - Status changed from 'new' to 'open'

Sun Feb 26 05:48:43 2017 SHLOMIF [...] cpan.org - Status changed from 'open' to 'stalled'

Sun Feb 26 10:46:03 2017 HVDS [...] cpan.org - Correspondence added

On Sun Feb 26 05:48:37 2017, SHLOMIF wrote: Show quoted text

> Hi Hugo, > > On Sat Feb 18 09:03:40 2017, HVDS wrote:

> > % perl -MMath::GMP -we 'my $x; $x += 1' > > % perl -MMath::GMP -we 'my $x; $x += Math::GMP->new(1)' > > Use of uninitialized value in null operation at -e line 1. > > %

> > The more idiomatic and recommended way would be to do "my $x = 0; $x > += ..."

I think I disagree with this: it doesn't warn with plain integers precisely because this _is_ idiomatic perl, particulary in the context of accumulating things in a hash (the context in which I spotted this) - when converting my working code to use Math::GMP instead of plain ints, I ended up having to replace this: $hash{$new_key} += $count; with this: ($hash{$new_key} //= Math::Gmp->new(0)) += $count; .. which is harder to read, and unncessarily slower. Show quoted text

> so the warning may actually be a good thing.

Even if _a_ warning were desirable, a warning that it's "in null operation" certainly is not the ideal one. Hugo

Sun Feb 26 10:46:04 2017 The RT System itself - Status changed from 'stalled' to 'open'

Sun Feb 26 11:23:42 2017 SHLOMIF [...] cpan.org - Correspondence added

On Sun Feb 26 10:46:03 2017, HVDS wrote: Show quoted text

> On Sun Feb 26 05:48:37 2017, SHLOMIF wrote:

> > Hi Hugo, > > > > On Sat Feb 18 09:03:40 2017, HVDS wrote:

> > > % perl -MMath::GMP -we 'my $x; $x += 1' > > > % perl -MMath::GMP -we 'my $x; $x += Math::GMP->new(1)' > > > Use of uninitialized value in null operation at -e line 1. > > > %

> > > > The more idiomatic and recommended way would be to do "my $x = 0; $x > > += ..."

> > I think I disagree with this: it doesn't warn with plain integers > precisely because this _is_ idiomatic perl, particulary in the context > of accumulating things in a hash (the context in which I spotted this) > - when converting my working code to use Math::GMP instead of plain > ints, I ended up having to replace this: > $hash{$new_key} += $count; > with this: > ($hash{$new_key} //= Math::Gmp->new(0)) += $count; > .. which is harder to read, and unncessarily slower. >

Ah, I guess you're right about this use with hashes. Show quoted text

> > so the warning may actually be a good thing.

> > Even if _a_ warning were desirable, a warning that it's "in null > operation" certainly is not the ideal one. > > Hugo

Tue Nov 21 14:32:00 2017 SHLOMIF [...] cpan.org - Correspondence added

On Sun Feb 26 11:23:42 2017, SHLOMIF wrote: Show quoted text

> On Sun Feb 26 10:46:03 2017, HVDS wrote:

> > On Sun Feb 26 05:48:37 2017, SHLOMIF wrote:

> > > Hi Hugo, > > > > > > On Sat Feb 18 09:03:40 2017, HVDS wrote:

> > > > % perl -MMath::GMP -we 'my $x; $x += 1' > > > > % perl -MMath::GMP -we 'my $x; $x += Math::GMP->new(1)' > > > > Use of uninitialized value in null operation at -e line 1. > > > > %

> > > > > > The more idiomatic and recommended way would be to do "my $x = 0; $x > > > += ..."

> > > > I think I disagree with this: it doesn't warn with plain integers > > precisely because this _is_ idiomatic perl, particulary in the context > > of accumulating things in a hash (the context in which I spotted this) > > - when converting my working code to use Math::GMP instead of plain > > ints, I ended up having to replace this: > > $hash{$new_key} += $count; > > with this: > > ($hash{$new_key} //= Math::Gmp->new(0)) += $count; > > .. which is harder to read, and unncessarily slower. > >

> > Ah, I guess you're right about this use with hashes. >

marking as stalled again, not because I disagree but to avoid this cluttering rt.cpan.org.

Tue Nov 21 14:32:01 2017 SHLOMIF [...] cpan.org - Status changed from 'open' to 'stalled'