Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fwd: [PATCH] hv_iterinit return value documented wrong? #13655

Closed
p5pRT opened this issue Mar 11, 2014 · 14 comments
Closed

Fwd: [PATCH] hv_iterinit return value documented wrong? #13655

p5pRT opened this issue Mar 11, 2014 · 14 comments

Comments

@p5pRT
Copy link

p5pRT commented Mar 11, 2014

Migrated from rt.perl.org#121418 (status was 'resolved')

Searchable as RT121418$

@p5pRT
Copy link
Author

p5pRT commented Mar 11, 2014

From @wolfsage

Created by wolfsage@gmail.com

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)

Perl Info

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

@p5pRT
Copy link
Author

p5pRT commented Mar 11, 2014

From @wolfsage

0001-Correct-hv_iterinit-s-return-value-documentation.patch
From df0cffa2368c83c09f63fd7642499032232e9607 Mon Sep 17 00:00:00 2001
From: Matthew Horsfall <WolfSage@gmail.com>
Date: Tue, 11 Mar 2014 15:59:15 -0400
Subject: [PATCH] Correct hv_iterinit's return value documentation

---
 hv.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hv.c b/hv.c
index 9d80659..8aa3ddd 100644
--- a/hv.c
+++ b/hv.c
@@ -1989,8 +1989,8 @@ S_hv_auxinit(pTHX_ HV *hv) {
 =for apidoc hv_iterinit
 
 Prepares a starting point to traverse a hash table.  Returns the number of
-keys in the hash (i.e. the same as C<HvUSEDKEYS(hv)>).  The return value is
-currently only meaningful for hashes without tie magic.
+keys in the hash, including placeholders (i.e. the same as C<HvTOTALKEYS(hv)>).
+The return value is currently only meaningful for hashes without tie magic.
 
 NOTE: Before version 5.004_65, C<hv_iterinit> used to return the number of
 hash buckets that happen to be in use.  If you still need that esoteric
-- 
1.7.9.5

@p5pRT
Copy link
Author

p5pRT commented Mar 11, 2014

From @wolfsage

0001-Update-iterinit-s-return-to-match-documentation.patch
From 9e23b36859e64ea3461b1600e4f0ac5cf1db5a4c Mon Sep 17 00:00:00 2001
From: Matthew Horsfall <WolfSage@gmail.com>
Date: Tue, 11 Mar 2014 16:13:48 -0400
Subject: [PATCH] Update iterinit's return to match documentation

---
 hv.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hv.c b/hv.c
index 9d80659..a660e2c 100644
--- a/hv.c
+++ b/hv.c
@@ -2028,7 +2028,7 @@ Perl_hv_iterinit(pTHX_ HV *hv)
     }
 
     /* used to be xhv->xhv_fill before 5.004_65 */
-    return HvTOTALKEYS(hv);
+    return HvUSEDKEYS(hv);
 }
 
 I32 *
-- 
1.7.9.5

@p5pRT
Copy link
Author

p5pRT commented Mar 11, 2014

From @bulk88

- return HvTOTALKEYS(hv);
+ return HvUSEDKEYS(hv);

the 2 macros aren't the same, idk enough to comment further

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Mar 11, 2014

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Mar 12, 2014

From @iabyn

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.

--
Overhead, without any fuss, the stars were going out.
  -- Arthur C Clarke

@p5pRT
Copy link
Author

p5pRT commented Feb 26, 2017

From @jkeenan

On Wed, 12 Mar 2014 20​:49​:38 GMT, davem wrote​:

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)

@p5pRT
Copy link
Author

p5pRT commented Feb 27, 2017

From @wolfsage

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.

Whether or not before 5.26.0 I leave to Sawyer though.

-- Matthew Horsfall (alh)

@p5pRT
Copy link
Author

p5pRT commented Feb 28, 2017

From @iabyn

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.

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

@p5pRT
Copy link
Author

p5pRT commented Feb 28, 2017

From @xsawyerx

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.

@p5pRT
Copy link
Author

p5pRT commented Feb 28, 2017

From @jkeenan

On Tue, 28 Feb 2017 09​:49​:48 GMT, xsawyerx@​gmail.com wrote​:

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 fe7d7ed. Marking ticket Resolved.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Feb 28, 2017

@jkeenan - Status changed from 'open' to 'resolved'

@p5pRT p5pRT closed this as completed Feb 28, 2017
@p5pRT
Copy link
Author

p5pRT commented Feb 28, 2017

From @wolfsage

Thanks!

-- Matthew Horsfall (alh)

@p5pRT
Copy link
Author

p5pRT commented Feb 28, 2017

From @khwilliamson

On 02/28/2017 02​:48 AM, Sawyer X wrote​:

That's good. Contradicting oneself is a fickle situation

unless you're a politician

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant