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

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

Thread Previous | Thread Next
From:
Jeffrey Friedl
Date:
October 31, 2001 08:25
Subject:
Re: [PATCH] core-only patch for clamp/readonly hashes
Message ID:
200110311624.f9VGOSL56077@ventrue.corp.yahoo.com

Nick Ing-Simmons <nick.ing-simmons@elixent.com> wrote:
|> >+=item Can't access nonexistant key {%s} of readonly/clamped hash
|> 
|> That "clamped" word has IMHO got to go ...

As I've said a number of times, I don't care what you call it.
Call it "HD" (for "humpty dumpty") for all I care.
For discussion, I had to pick a name and I picked "clamp".


|> >+(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.

Okay, here goes, for the umpteenth time. It's an expanding set of ideas:

1) If a hash's set of keys are readonly, as evidence by SvREADONLY(hv),
   keys may not be added nor subtracted. (It is not, however, an error to
   *access* a non-existent key.)

2) An additional feature would be to disallow access to a non-existent key,
   such that
        if (not $self->{Quite}) {
           warn "..."
        }
   would produce a fatal runtime error if 'Quite' didn't exist in the hash
   referenced by $self. It is, though, always okay to use exists() on any key.

3) Finally, an additional idea would be to expand the notion of the
   ``safe-working set'' of allowed keys (keys that can exist, be added,
   deleted, and accessed) to be a specific list of keys. This means that if
   a key is a memeber of the specific list, it may be added, deleted, and
   accessed at will.

   If it is not a member of the list, it may not be added or deleted, so
   this functionality is a subset of readonly.

   If it is not a member of the list, it is an error to access the key in
   any way except exists(). However, if the key is a member of the list,
   it is okay to access it even though it doesn't exist().

Here's an example: Let's say that I'm doing the ever-popular CD database,
and want to have a hash represent a track. I might create the hash, and
give it this list of possible keys:

    TrackID
    Artist
    Title
    DiskID
    Length
    Comments

The various data-entry parts of the program would allow those fields, or at
least some of them, to be filled in. If, then, one part of the program
tried to access {TrackId} instead of {TrackID}, it would be an error.

So, I think of this new feature as one that puts limits on either side of
my mental list of keys I want to allow, holding them fixed (as if in a vice).
So, perhaps now you can see where I came up with the name. But again,
I don't care about the name.

Now, for implementation:

   The keys that exist in a hash when it is
   clamped/frozen/HD'd/restricted/whatever are the keys that are "allowed".
   You can then delete the keys you don't actually want, but others are
   free to access/add/delete any of the original set.


|> 
|> 
|> >     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.

No problem. As I said in a note following the patch, I figured out a way do
deal with it in a way that made it (whether the count is IV or NV) not
really matter.

|> > #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?

If you search the entire code (not just the patch) for where xhv_keys and
HvKEYS are used, you'll see situations where one (keys) or the other
(realkeys) is required. I felt it safer to leave the logical "total number
of element structs allocated in this hash" number in its original location,
since that is used for memory allocation, etc. Other than this issue,
it's a six-of-one-or-half-a-dozen-of-the-other issues, so it jus seemed safer
to take the prudent route.


|> >+
|> >+#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 

The semantics have been explicitly, clearly defined, Nick. Perhaps you've
forgotten the discussions that you participated in in July and August?
Perhaps you didn't read the documentation in the patch I sent yesterday?

In any case, I hope the description earlier in this note is sufficient to
jog your memory.


|> > #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 ?

I wanted to tread light, so picked what I thought was safe.
I'll use whatever value you want me to use.

What value should be used?

|> > 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.

Sigh.  It's not pointless at all Nick. It's needed.

I don't mind that you don't understand the full scope of what I've done,
but if you choose to criticize, I do wish you'd understand first.


|> >-    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...

Sigh. Yes, that's true.
But then *other* places would need to be changed.

|> >--- 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.

You can see from the comment what I intended. Can you suggest a way around
whatever problem you see in the implementation? Or, at least, what the
exact problem is. As I'm sure is clear, I don't have the breadth of core
understanding that most on p5p have.

Thanks,
        Jeffrey

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