Skip Menu |
Report information
Id: 121418
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: alh <wolfsage [at] gmail.com>
Cc:
AdminCc:

Operating System: Linux
PatchStatus: HasPatch
Severity: low
Type:
Perl Version: 5.14.2
Fixed In: 5.25.11

Attachments
0001-Correct-hv_iterinit-s-return-value-documentation.patch
0001-Update-iterinit-s-return-to-match-documentation.patch



From: "Matthew Horsfall (alh)" <wolfsage [...] gmail.com>
Date: Tue, 11 Mar 2014 16:16:49 -0400
To: perlbug [...] perl.org
Subject: Fwd: [PATCH] hv_iterinit return value documented wrong?
Download (untitled) / with headers
text/plain 5.2k
Whoops... Off to perlbug Show quoted text
---------- Forwarded message ---------- From: Matthew Horsfall (alh) <wolfsage@gmail.com> Date: Tue, Mar 11, 2014 at 4:15 PM Subject: [PATCH] hv_iterinit return value documented wrong? To: Perl5 Porters <perl5-porters@perl.org> This is a bug report for perl from wolfsage@gmail.com, generated with the help of perlbug 1.39 running under perl 5.14.2. ----------------------------------------------------------------- [Please describe your issue here] hv_iterinit's docs say: hv_iterinit Prepares a starting point to traverse a hash table. Returns the number of keys in the hash (i.e. the same as "HvUSEDKEYS(hv)"). But it actually returns HvTOTALKEYS(hv). This can differ, as shown by the example here: https://github.com/wolfsage/scratch/tree/master/Test-Keys Specifically, Test-Keys.xs: void printit(hv) SV *hv; PPCODE: printf("TOTAL: %d\nUSED: %d\nITER: %d\n", HvTOTALKEYS((HV*)SvRV(hv)), HvUSEDKEYS((HV*)SvRV(hv)), hv_iterinit((HV*)SvRV(hv)) ); And in Perl: use Test::Keys; use Hash::Util qw(lock_keys); my %hash = qw(cat dog mouse bird); lock_keys(%hash); %hash = (); Test::Keys::printit(\%hash); Which outputs: TOTAL: 2 USED: 0 ITER: 2 Attached are two patches with two different attempts at fixing. The first updates the docs to say HvTOTALKEYS instead, with a little more info. The second instead updates the code to use HvUSEDKEYS instead, to be in line with the docs. (All tests pass with this change - so regardless of what we do here we should probably add a test case for this) Is either of these correct? Thanks, -- Matthew Horsfall (alh) (Thanks to stevan and bulk88 - noticed this because of them) [Please do not change anything below this line] ----------------------------------------------------------------- --- Flags: category=core severity=low --- Site configuration information for perl 5.14.2: Configured by Debian Project at Mon Mar 18 19:16:26 UTC 2013. Summary of my perl5 (revision 5 version 14 subversion 2) configuration: Platform: osname=linux, osvers=2.6.42-37-generic, archname=x86_64-linux-gnu-thread-multi uname='linux batsu 2.6.42-37-generic #58-ubuntu smp thu jan 24 15:28:10 utc 2013 x86_64 x86_64 x86_64 gnulinux ' config_args='-Dusethreads -Duselargefiles -Dccflags=-DDEBIAN -Dcccdlflags=-fPIC -Darchname=x86_64-linux-gnu -Dprefix=/usr -Dprivlib=/usr/share/perl/5.14 -Darchlib=/usr/lib/perl/5.14 -Dvendorprefix=/usr -Dvendorlib=/usr/share/perl5 -Dvendorarch=/usr/lib/perl5 -Dsiteprefix=/usr/local -Dsitelib=/usr/local/share/perl/5.14.2 -Dsitearch=/usr/local/lib/perl/5.14.2 -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 -DDEBUGGING=-g -Doptimize=-O2 -Duseshrplib -Dlibperl=libperl.so.5.14.2 -des' hint=recommended, useposix=true, d_sigaction=define useithreads=define, usemultiplicity=define useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef use64bitint=define, use64bitall=define, uselongdouble=undef usemymalloc=n, bincompat5005=undef Compiler: cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64', optimize='-O2 -g', cppflags='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include' ccversion='', gccversion='4.6.3', gccosandvers='' intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678 d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16 ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8 alignbytes=8, prototype=define Linker and Libraries: ld='cc', ldflags =' -fstack-protector -L/usr/local/lib' libpth=/usr/local/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib /usr/lib libs=-lgdbm -lgdbm_compat -ldb -ldl -lm -lpthread -lc -lcrypt perllibs=-ldl -lm -lpthread -lc -lcrypt libc=, so=so, useshrplib=true, libperl=libperl.so.5.14.2 gnulibc_version='2.15' Dynamic Linking: dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E' cccdlflags='-fPIC', lddlflags='-shared -O2 -g -L/usr/local/lib -fstack-protector' Locally applied patches: --- @INC for perl 5.14.2: /etc/perl /usr/local/lib/perl/5.14.2 /usr/local/share/perl/5.14.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.14 /usr/share/perl/5.14 /usr/local/lib/site_perl . --- Environment for perl 5.14.2: HOME=/home/mhorsfall LANG=en_US.UTF-8 LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/home/mhorsfall/perl5/perlbrew/bin:/home/mhorsfall/bin:/home/mhorsfall/bin:/usr/lib/lightdm/lightdm:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games PERLBREW_BASHRC_VERSION=0.66 PERLBREW_HOME=/home/mhorsfall/.perlbrew PERLBREW_ROOT=/home/mhorsfall/perl5/perlbrew PERL_BADLANG (unset) SHELL=/bin/bash

Message body is not shown because sender requested not to inline it.

Message body is not shown because sender requested not to inline it.

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 154b
- return HvTOTALKEYS(hv); + return HvUSEDKEYS(hv); the 2 macros aren't the same, idk enough to comment further -- bulk88 ~ bulk88 at hotmail.com
To: perl5-porters [...] perl.org
Date: Wed, 12 Mar 2014 20:49:18 +0000
Subject: Re: [perl #121418] Fwd: [PATCH] hv_iterinit return value documented wrong?
From: Dave Mitchell <davem [...] iabyn.com>
Download (untitled) / with headers
text/plain 860b
On Tue, Mar 11, 2014 at 01:17:04PM -0700, Matthew Horsfall wrote: Show quoted text
> hv_iterinit's docs say: > > hv_iterinit > Prepares a starting point to traverse a hash table. Returns > the number of keys in the hash (i.e. the same as > "HvUSEDKEYS(hv)"). > > But it actually returns HvTOTALKEYS(hv).
The only use of its return value in core is as a boolean: i.e. it's used to determine whether iterating will never return any values, and thus never bother calling hv_iternext(). Now, since hv_iternext_flags() can indicate via a flag whether or not to skip over placeholders, I think the most conservative approach is to include placeholders in the returned count; i.e. return HvTOTALKEYS(). So I conclude that the docs need fixing. -- Overhead, without any fuss, the stars were going out. -- Arthur C Clarke
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.1k
On Wed, 12 Mar 2014 20:49:38 GMT, davem wrote: Show quoted text
> On Tue, Mar 11, 2014 at 01:17:04PM -0700, Matthew Horsfall wrote:
> > hv_iterinit's docs say: > > > > hv_iterinit > > Prepares a starting point to traverse a hash table. Returns > > the number of keys in the hash (i.e. the same as > > "HvUSEDKEYS(hv)"). > > > > But it actually returns HvTOTALKEYS(hv).
> > The only use of its return value in core is as a boolean: i.e. it's > used > to determine whether iterating will never return any values, and thus > never bother calling hv_iternext(). Now, since hv_iternext_flags() can > indicate via a flag whether or not to skip over placeholders, I think > the > most conservative approach is to include placeholders in the returned > count; i.e. return HvTOTALKEYS(). > > So I conclude that the docs need fixing.
Can we get an update on the status of the patches originally submitted by Matt in this RT? The first patch is documentation-only (though the location of that documentation within the file has changed). The second is a C-level source code change (which I think we would have to defer until after code freeze). Thank you very much. -- James E Keenan (jkeenan@cpan.org)
Date: Mon, 27 Feb 2017 09:30:30 -0500
To: perlbug-followup [...] perl.org
From: "Matthew Horsfall (alh)" <wolfsage [...] gmail.com>
Subject: Re: [perl #121418] Fwd: [PATCH] hv_iterinit return value documented wrong?
Download (untitled) / with headers
text/plain 533b
On Sun, Feb 26, 2017 at 5:46 PM, James E Keenan via RT <perlbug-followup@perl.org> wrote: Show quoted text
> The first patch is documentation-only (though the location of that documentation within the file has changed). > > The second is a C-level source code change (which I think we would have to defer until after code freeze).
Dave believes the docs need fixing. With that being the case, we could apply the doc fix and throw out the C-level code change patch. Whether or not before 5.26.0 I leave to Sawyer though. -- Matthew Horsfall (alh)
From: Dave Mitchell <davem [...] iabyn.com>
Subject: Re: [perl #121418] Fwd: [PATCH] hv_iterinit return value documented wrong?
To: "Matthew Horsfall (alh)" <wolfsage [...] gmail.com>
CC: perlbug-followup [...] perl.org
Date: Tue, 28 Feb 2017 08:30:44 +0000
Download (untitled) / with headers
text/plain 985b
On Mon, Feb 27, 2017 at 09:30:30AM -0500, Matthew Horsfall (alh) wrote: Show quoted text
> On Sun, Feb 26, 2017 at 5:46 PM, James E Keenan via RT > <perlbug-followup@perl.org> wrote:
> > The first patch is documentation-only (though the location of that documentation within the file has changed). > > > > The second is a C-level source code change (which I think we would have to defer until after code freeze).
> > Dave believes the docs need fixing. With that being the case, we could > apply the doc fix and throw out the C-level code change patch.
Having now had a quick at the history, that function has been returning total keys since 2001, so I agree with myself(!) that its the docs that should be fixed and not the function. I'm happy for the doc patch to be applied before 5.26.0. -- Wesley Crusher gets beaten up by his classmates for being a smarmy git, and consequently has a go at making some friends of his own age for a change. -- Things That Never Happen in "Star Trek" #18
Subject: Re: [perl #121418] Fwd: [PATCH] hv_iterinit return value documented wrong?
To: Dave Mitchell <davem [...] iabyn.com>, "Matthew Horsfall (alh)" <wolfsage [...] gmail.com>
Date: Tue, 28 Feb 2017 10:48:50 +0100
CC: perlbug-followup [...] perl.org
From: Sawyer X <xsawyerx [...] gmail.com>
Download (untitled) / with headers
text/plain 928b
On 02/28/2017 09:30 AM, Dave Mitchell wrote: Show quoted text
> On Mon, Feb 27, 2017 at 09:30:30AM -0500, Matthew Horsfall (alh) wrote:
>> On Sun, Feb 26, 2017 at 5:46 PM, James E Keenan via RT >> <perlbug-followup@perl.org> wrote:
>>> The first patch is documentation-only (though the location of that documentation within the file has changed). >>> >>> The second is a C-level source code change (which I think we would have to defer until after code freeze).
>> Dave believes the docs need fixing. With that being the case, we could >> apply the doc fix and throw out the C-level code change patch.
> Having now had a quick at the history, that function has been returning > total keys since 2001, so I agree with myself(!) that its the docs that > should be fixed and not the function.
That's good. Contradicting oneself is a fickle situation. :) Show quoted text
> > I'm happy for the doc patch to be applied before 5.26.0.
Yes, please. Thanks guys.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.3k
On Tue, 28 Feb 2017 09:49:48 GMT, xsawyerx@gmail.com wrote: Show quoted text
> > > On 02/28/2017 09:30 AM, Dave Mitchell wrote:
> > On Mon, Feb 27, 2017 at 09:30:30AM -0500, Matthew Horsfall (alh) > > wrote:
> >> On Sun, Feb 26, 2017 at 5:46 PM, James E Keenan via RT > >> <perlbug-followup@perl.org> wrote:
> >>> The first patch is documentation-only (though the location of that > >>> documentation within the file has changed). > >>> > >>> The second is a C-level source code change (which I think we would > >>> have to defer until after code freeze).
> >> Dave believes the docs need fixing. With that being the case, we > >> could > >> apply the doc fix and throw out the C-level code change patch.
> > Having now had a quick at the history, that function has been > > returning > > total keys since 2001, so I agree with myself(!) that its the docs > > that > > should be fixed and not the function.
> > That's good. Contradicting oneself is a fickle situation. :) >
> > > > I'm happy for the doc patch to be applied before 5.26.0.
> > Yes, please. > > Thanks guys.
Consensus appears to be: Apply the doc patch now; throw out the code change. (If we have second thoughts on the latter, we can open a new RT.) Doc patch applied in commit fe7d7ed30f61366a4cfdd0edc6051ffc736a572a. Marking ticket Resolved. Thank you very much. -- James E Keenan (jkeenan@cpan.org)
Subject: Re: [perl #121418] Fwd: [PATCH] hv_iterinit return value documented wrong?
From: "Matthew Horsfall (alh)" <wolfsage [...] gmail.com>
To: perlbug-followup [...] perl.org
Date: Tue, 28 Feb 2017 08:16:54 -0500
CC: Perl5 Porters <perl5-porters [...] perl.org>
Thanks! -- Matthew Horsfall (alh)
Date: Tue, 28 Feb 2017 09:11:42 -0700
CC: perlbug-followup [...] perl.org
Subject: Re: [perl #121418] Fwd: [PATCH] hv_iterinit return value documented wrong?
From: Karl Williamson <public [...] khwilliamson.com>
To: Sawyer X <xsawyerx [...] gmail.com>, Dave Mitchell <davem [...] iabyn.com>, "Matthew Horsfall (alh)" <wolfsage [...] gmail.com>
Download (untitled) / with headers
text/plain 128b
On 02/28/2017 02:48 AM, Sawyer X wrote: Show quoted text
> That's good. Contradicting oneself is a fickle situation
unless you're a politician


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org