develooper Front page | perl.perl5.porters | Postings from October 2001

Re: [PATCH] core-only patch for clamp/readonly hashes

Thread Previous | Thread Next
From:
Nick Ing-Simmons
Date:
October 31, 2001 06:59
Subject:
Re: [PATCH] core-only patch for clamp/readonly hashes
Message ID:
20011031145254.12311.1@bactrian.elixent.com
Jeffrey Friedl <jfriedl@yahoo.com> writes:

Here are my comments having read it:

>
>+=item Can't access nonexistant key {%s} of readonly/clamped hash

That "clamped" word has IMHO got to go ...

>+
>+(F) You attempted to access a nonexistant key of a hash that has been marked
>+readonly, or has had its set of keys clamped (frozen). You can always check
>+if a key C<exists()>, but other kinds of access of such keys are errors.

frozen makes a little more sense perhaps but I am still unclear what the
specification is. Author and I yesterday described what semantics
of SvREADONLY would be. We did not describe what "frozen" would be.
I still don't understand the semantics of "frozen".

Please describe the semantics of "frozen" (don't used the clamped word)
explaining how it differs from READONLY.



>     STRLEN	xhv_max;	/* subscript of last element of xhv_array */
>-    IV		xhv_keys;	/* how many elements in the array */
>-    NV		xnv_nv;		/* numeric value, if any */
>+    IV		xhv_realkeys;	/* elements in the array (excl placeholders) */
>+    union {
>+      NV		_nv;	/* numeric value, if any (unused)*/
>+      IV                _keys;  /* elements in the array (incl placeholders) */
>+    } xhv_u;
>+#define xhv_keys xhv_u._keys
>     MAGIC*	xmg_magic;	/* magic for scalar array */
>     HV*		xmg_stash;	/* class package */

We are not going to add the union. As soon as you add it for HV then
offset of xmg_magic may not be the same as in other SV-oid types.

> #define HvKEYS(hv)	((XPVHV*)  SvANY(hv))->xhv_keys
>+#define HvREALKEYS(hv)	((XPVHV*)  SvANY(hv))->xhv_realkeys

Fair enough but why has the one with old meaning moved?

>+
>+#define HvCLAMPEDACCESS(hv)	(SvFLAGS(hv) &   SVh_CLAMP_ACCESS)
>+#define HvCLAMPEDACCESS_on(hv)	(SvFLAGS(hv) |=  SVh_CLAMP_ACCESS)
>+#define HvCLAMPEDACCESS_off(hv)	(SvFLAGS(hv) &= ~SVh_CLAMP_ACCESS)

I am still worried about using the bit for a very ill-defined
semantics - if I knew how clamped/frozen/fixed HV differed from
a readonly HV I might be convinced we need the bit.

>
> #define HvSHAREKEYS(hv)		(SvFLAGS(hv) & SVphv_SHAREKEYS)
> #define HvSHAREKEYS_on(hv)	(SvFLAGS(hv) |= SVphv_SHAREKEYS)
>--- bleedperl.orig/sv.h	Sat Oct 27 19:37:31 2001
>+++ bleedperl/sv.h	Tue Oct 30 08:38:46 2001
>@@ -234,6 +234,8 @@
>
> #define SVprv_WEAKREF   0x80000000      /* Weak reference */
>
>+#define SVh_CLAMP_ACCESS 0x08000000	/* hash access has been clamped */

If we have the bit can we group it in sequence between
0x10000000 and 0x04000000 ?

>
> NOTE: Before version 5.004_65, C<hv_iterinit> used to return the number of
>@@ -1412,7 +1617,7 @@
>     xhv->xhv_riter = -1; 	/* HvRITER(hv) = -1 */
>     xhv->xhv_eiter = Null(HE*); /* HvEITER(hv) = Null(HE*) */
>     /* used to be xhv->xhv_fill before 5.004_65 */
>-    return xhv->xhv_keys; /* HvKEYS(hv) */
>+    return xhv->xhv_realkeys; /* HvREALKEYS(hv) */
> }

One of several "pointless" changes - if you had kept the name/field
xhv_keys for that use and used a new name for the other.

>--- bleedperl.orig/doop.c	Mon Sep  3 09:18:09 2001
>+++ bleedperl/doop.c	Mon Oct 29 22:00:47 2001
>@@ -1303,7 +1303,7 @@
> 	}
>
> 	if (! SvTIED_mg((SV*)keys, PERL_MAGIC_tied))
>-	    i = HvKEYS(keys);
>+	    i = HvREALKEYS(keys);
> 	else {
> 	    i = 0;
> 	    /*SUPPRESS 560*/
>@@ -1313,7 +1313,7 @@
> 	RETURN;
>     }
>
>-    EXTEND(SP, HvKEYS(keys) * (dokeys + dovalues));
>+    EXTEND(SP, HvREALKEYS(keys) * (dokeys + dovalues));
>
>     PUTBACK;	/* hv_iternext and hv_iterval might clobber stack_sp */
>     while ((entry = hv_iternext(keys))) {

With a suitable non-change of name doop.c would not need to be touched...



>--- bleedperl.orig/scope.c	Fri Oct 26 07:09:53 2001
>+++ bleedperl/scope.c	Mon Oct 29 22:05:44 2001
>@@ -169,7 +169,7 @@
>     while (PL_tmps_ix > myfloor) {      /* clean up after last statement */
> 	SV* sv = PL_tmps_stack[PL_tmps_ix];
> 	PL_tmps_stack[PL_tmps_ix--] = Nullsv;
>-	if (sv) {
>+	if (sv && sv != &PL_sv_undef) {
> 	    SvTEMP_off(sv);
> 	    SvREFCNT_dec(sv);		/* note, can modify tmps_ix!!! */
> 	}
>@@ -823,8 +823,18 @@
> 	    sv = *(SV**)ptr;
> 	    /* Can clear pad variable in place? */
> 	    if (SvREFCNT(sv) <= 1 && !SvOBJECT(sv)) {
>+		/*
>+		 * if a my variable that was made readonly is going out of
>+		 * scope, we want to remove the readonlyness so that it can
>+		 * go out of scope quietly
>+		 */
>+		if (SvPADMY(sv))
>+		    SvREADONLY_off(sv);
>+

Whoa there - you leave my READONLY variables alone - in particular
READONLY+FAKE means something - blindly turning off READONLY is
not a good idea.

--
Nick Ing-Simmons
http://www.ni-s.u-net.com/



Thread Previous | Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About