Re: [perl #118139] Storable in DESTROY blocks

Front page | perl.perl5.porters | Postings from May 2013

Re: [perl #118139] Storable in DESTROY blocks

Thread Previous

From:

Nicholas Clark

Date:

May 29, 2013 11:05

Subject:

Re: [perl #118139] Storable in DESTROY blocks

Message ID:

20130529110522.GR3729@plum.flirble.org

On Thu, May 23, 2013 at 09:42:29AM -0700, Reini Urban via RT wrote:
> Attached is a revised patch. 

> @@ -2259,7 +2273,16 @@ sortcmp(const void *a, const void *b)
>  static int store_hash(pTHX_ stcxt_t *cxt, HV *hv)
>  {
>  	dVAR;
> -	I32 len = HvTOTALKEYS(hv);
> +	I32 len =
> +#if PERL_VERSION < 8
> +             HvKEYS(hv);
> +#else
> +#  ifdef HAS_RESTRICTED_HASHES
> +             HvTOTALKEYS(hv);
> +#  else
> +             HvUSEDKEYS(hv);
> +#  endif
> +#endif
>  	I32 i;
>  	int ret = 0;
>  	I32 riter;

I don't think that change does anything. For starters, HAS_RESTRICTED_HASHES
will always be true if PERL_VERSION >= 8, because it's defined earlier by:

#ifdef HvPLACEHOLDERS
#define HAS_RESTRICTED_HASHES
#else

and HvPLACEHOLDERS was added by commit 8aacddc1ea3837f8 back in 5.7.2

So that simplifies to


-	I32 len = HvTOTALKEYS(hv);
+	I32 len =
+#if PERL_VERSION < 8
+             HvKEYS(hv);
+#else
+             HvTOTALKEYS(hv);
+#endif


In turn, Storable.xs already does this for compatibility with 5.6.x and
earlier:

#ifndef HvTOTALKEYS
#  define HvTOTALKEYS(hv)	HvKEYS(hv)
#endif


and as HvTOTALKEYS was also first introduced by commit 8aacddc1ea3837f8,
for 5.6 and earlier it's not defied, so Storable.xs makes that fallback.

Hence for those versions, HvTOTALKEYS(hv) is equivalent to HvKEYS(). Which
means that the original 1 line is identical to the changed version.

-	I32 len = HvTOTALKEYS(hv);
+	I32 len =
+#if PERL_VERSION < 8
+             HvKEYS(hv);
+#else
+             HvTOTALKEYS(hv);
+#endif


Here's the difference in pre-processor output for 5.6.2, 5.8.0 and 5.8.9 with
and without that patch hunk:

--- Storable.i.562      2013-05-29 12:50:56.000000000 +0200
+++ Storable.i.562.patched      2013-05-29 12:50:34.000000000 +0200
@@ -7945,7 +7945,16 @@
 static int store_hash( stcxt_t *cxt, HV *hv)
 {
  extern int Perl___notused;
- I32 len = ((XPVHV*) (hv)->sv_any)->xhv_keys;
+ I32 len =
+
+             ((XPVHV*) (hv)->sv_any)->xhv_keys;
+
+
+
+
+
+
+
  I32 i;
  int ret = 0;
  I32 riter;
@@ -7985,7 +7994,7 @@

--- Storable.i.580      2013-05-29 12:51:14.000000000 +0200
+++ Storable.i.580.patched      2013-05-29 12:50:08.000000000 +0200
@@ -8185,7 +8185,16 @@
 static int store_hash( stcxt_t *cxt, HV *hv)
 {
  extern int Perl___notused __attribute__((unused));
- I32 len = (((XPVHV*) (hv)->sv_any)->xhv_keys);
+ I32 len =
+
+
+
+
+             (((XPVHV*) (hv)->sv_any)->xhv_keys);
+
+
+
+
  I32 i;
  int ret = 0;
  I32 riter;

--- Storable.i.589      2013-05-29 12:51:36.000000000 +0200
+++ Storable.i.589.patched      2013-05-29 12:49:43.000000000 +0200
@@ -9126,7 +9126,16 @@
 static int store_hash( stcxt_t *cxt, HV *hv)
 {
  extern int Perl___notused __attribute__((unused));
- I32 len = (((XPVHV*) (hv)->sv_any)->xhv_keys);
+ I32 len =
+
+
+
+
+             (((XPVHV*) (hv)->sv_any)->xhv_keys);
+
+
+
+
  I32 i;
  int ret = 0;
  I32 riter;


Only whitespace differences.


I could build blead's current Storable.xs on 5.6.1 and 5.6.2 without any
changes. The XS is actually fine all the way back to 5.004
So I'm not sure why any additional 5.6.2 changes are needed.

Nicholas Clark

Thread Previous