Re: let unset array element remove compatible with bash

Re: let unset array element remove compatible with bash

[Bart Schaefer]

[Moved to -workers for discussion]

On Feb 22,  3:28am, Daniel Lin wrote:
}
} Can any developer consider to enhance zsh's function like "unset var[2]"?

Hmm, what's the reason NOT to do this?  I can think of one: "unset var[9]"
might cause elements 2 through 8 to spring into existence, which is surely
counter-intuitive ... but of course the same thing happens with var[9]=().

@@ -3049,8 +3063,14 @@
 		}
 		paramtab = tht;
 	    } else {
-		zerrnam(name, "%s: invalid element for unset", s);
-		returnval = 1;
+		char *arr[1]; arr[0] = 0;
+		*ss = '[';
+		if (assignaparam(s, zarrdup(arr), 0)) {
+		    returnval = 0;
+		} else {
+		    zerrnam(name, "%s: invalid element for unset", s);
+		    returnval = 1;
+		}
 	    }
 	} else {
 	    if (unsetparam_pm(pm, 0, 1))

Re: let unset array element remove compatible with bash

[Peter Stephenson]

On Tue, 21 Feb 2012 21:01:06 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Feb 22,  3:28am, Daniel Lin wrote:
> }
> } Can any developer consider to enhance zsh's function like "unset var[2]"?
> 
> Hmm, what's the reason NOT to do this?  I can think of one: "unset var[9]"
> might cause elements 2 through 8 to spring into existence, which is surely
> counter-intuitive ... but of course the same thing happens with var[9]=().

It should work, but it's not quite trivial:

$ array=(one three)
$ echo ${#array[*]}
2
$ unset 'array[8]'
$ echo ${#array[*]}
2

So it's not simply a case of unconditionally deleting an element.  I
think you need to check the element is present in the first place.

-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

Re: let unset array element remove compatible with bash

[Bart Schaefer]

On Feb 22,  9:52am, Peter Stephenson wrote:
}
} So it's not simply a case of unconditionally deleting an element.  I
} think you need to check the element is present in the first place.

Is there a straightforward way to do that?  The code in subst.c that
handles ${+var[x]} is rather convoluted, and every combination of
getvalue()/fetchvalue() that I've tried always returns non-NULL for
subscripted expressions.

Which probably explains this comment in subst.c:

    /*
     * Indicates we have tried to get a value in v but that was
     * unset.  I don't quite understand why (v == NULL) isn't
     * good enough, but there are places where we seem to need
     * to second guess whether a value is a real value or not.
     */
    int vunset = 0;

Then there's this horror, which appears to be fetching the entire
array to call arrlen() on it, just to discover whether one of the
elements is not set:

	    /* Value retrieved from parameter/subexpression is scalar */
	    if (v->pm->node.flags & PM_ARRAY) {
		/*
		 * Although the value is a scalar, the parameter
		 * itself is an array.  Presumably this is due to
		 * being quoted, or doing single substitution or something,
		 * TODO: we're about to do some definitely stringy
		 * stuff, so something like this bit is probably
		 * necessary.  However, I'd like to leave any
		 * necessary joining of arrays until this point
		 * to avoid the multsub() horror.
		 */
		int tmplen = arrlen(v->pm->gsu.a->getfn(v->pm));

		if (v->start < 0)
		    v->start += tmplen + ((v->flags & VALFLAG_INV) ? 1 : 0);
		if (!(v->flags & VALFLAG_INV) &&
		    (v->start >= tmplen || v->start < 0))
		    vunset = 1;
	    }

Gaah.

Re: let unset array element remove compatible with bash

[Peter Stephenson]

On Wed, 22 Feb 2012 09:28:27 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Feb 22,  9:52am, Peter Stephenson wrote:
> }
> } So it's not simply a case of unconditionally deleting an element.  I
> } think you need to check the element is present in the first place.
> 
> Is there a straightforward way to do that?  The code in subst.c that
> handles ${+var[x]} is rather convoluted, and every combination of
> getvalue()/fetchvalue() that I've tried always returns non-NULL for
> subscripted expressions.

At this level, I'd have thought it was good enough to test the length of
the array directly.  However, you can only do that after you've
evaluated the subscript, so it probably needs some poking down into
params.c first.

Presumably if you're trying to unset an element of the array, retrieving
it isn't likely to be difficult.
 
> Gaah.

Didn't I say that?

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

PATCH Re: let unset array element remove compatible with bash

[Bart Schaefer]

Revisiting this thread because I found an uncommitted diff in my local
sandbox.  I won't commit this pre-5.0 without further confirmation from
PWS.

On Feb 22,  8:19pm, Peter Stephenson wrote:
}
} On Wed, 22 Feb 2012 09:28:27 -0800
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > On Feb 22,  9:52am, Peter Stephenson wrote:
} > }
} > } So it's not simply a case of unconditionally deleting an element.  I
} > } think you need to check the element is present in the first place.
} > 
} > Is there a straightforward way to do that?  The code in subst.c that
} > handles ${+var[x]} is rather convoluted, and every combination of
} > getvalue()/fetchvalue() that I've tried always returns non-NULL for
} > subscripted expressions.
} 
} At this level, I'd have thought it was good enough to test the length of
} the array directly.  However, you can only do that after you've
} evaluated the subscript, so it probably needs some poking down into
} params.c first.
} 
} Presumably if you're trying to unset an element of the array, retrieving
} it isn't likely to be difficult.

So I stared at this a bit more.  Once I figured out that getvalue() did
in fact return enough information (in v->pm) to determine the length of
the array, it was easy to implement a test.

However, that was a bit unsatisfactory, because getvalue() would convert
the slice to be unset into its string value too, which is wasteful in
context.  Poking around in fetchvalue() reminded me of SCAPM_ARRONLY
which fetches the array without constructing a value for it.  Then it
was just a matter of cribbing the way fetchvalue() calls getindex().

So the patch below makes

    noglob unset foo[x]

work properly.  If x is an existing index (or valid slice), it is cut
from the array as if by foo[x]=().  Attempts to unset index values
outside the array, do nothing and return zero status.

As a bonus this will also unset slices of scalars.  If you try to
unset a slice of an integer or float, it returns a nonzero status but
doesn't error.  If we want an error here, then this should probably
also be an error:

torch% integer foo=123
torch% print $foo[2]
2
torch% foo[2]=4
torch% print $foo
4
torch% 

One thing I'm still scratching my head about:  In foo[(i)x] for scalar
foo, setstrvalue() doesn't properly apply the subscript range returned
from getindex().  That's why the PM_SCALAR branch below uses getvalue(),
which is somehow subtly different.  If someone can figure out how to
unify them, go for it.

Index: Src/builtin.c
--- ../zsh-forge/current/Src/builtin.c	2012-06-30 10:33:44.000000000 -0700
+++ Src/builtin.c	2012-07-01 09:33:55.000000000 -0700
@@ -3055,9 +3055,39 @@
 		    *sse = ']';
 		}
 		paramtab = tht;
+	    } else if (PM_TYPE(pm->node.flags) == PM_SCALAR) {
+		struct value vbuf;
+		Value v;
+		char *t = s;
+		*ss = '[';
+		if ((v = getvalue(&vbuf, &t, 1))) {
+		    setstrvalue(v, ztrdup(""));
+		}
+		returnval = errflag;
+		errflag = 0;
+	    } else if (PM_TYPE(pm->node.flags) == PM_ARRAY) {
+		struct value vbuf;
+		vbuf.isarr = SCANPM_ARRONLY;
+		vbuf.pm = pm;
+		vbuf.flags = 0;
+		vbuf.start = 0;
+		vbuf.end = -1;
+		vbuf.arr = 0;
+		*ss = '[';
+		if (getindex(&ss, &vbuf, SCANPM_ASSIGNING) == 0 &&
+		    vbuf.pm && !(vbuf.pm->node.flags & PM_UNSET)) {
+		    /* start is after the element for reverse index */
+		    int start = vbuf.start - !!(vbuf.flags & VALFLAG_INV);
+		    if (start < arrlen(vbuf.pm->u.arr)) {
+			char *arr[1];
+			arr[0] = 0;
+			setarrvalue(&vbuf, zarrdup(arr));
+		    }
+		}
+		returnval = errflag;
+		errflag = 0;
 	    } else {
-		zerrnam(name, "%s: invalid element for unset", s);
-		returnval = 1;
+	      returnval = 1;
 	    }
 	} else {
 	    if (unsetparam_pm(pm, 0, 1))

-- 
Barton E. Schaefer

Re: PATCH Re: let unset array element remove compatible with bash

[Peter Stephenson]

On Sun, 01 Jul 2012 09:53:54 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> So the patch below makes
> 
>     noglob unset foo[x]
> 
> work properly.  If x is an existing index (or valid slice), it is cut
> from the array as if by foo[x]=().

Do you mean that?  That's not how bash works, it doesn't cut the
element, just remove the value at that index (ignore the offset-zero
vs. offset-one difference, of course):

[pws@pws-pc tmp]$ foo=(one two three)
[pws@pws-pc tmp]$ echo ${foo[*]}
one two three
[pws@pws-pc tmp]$ echo ${foo[0]}
one
[pws@pws-pc tmp]$ echo ${foo[2]}
three
[pws@pws-pc tmp]$ unset 'foo[1]'
[pws@pws-pc tmp]$ echo ${foo[1]}

[pws@pws-pc tmp]$ echo ${foo[2]}
three

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: PATCH Re: let unset array element remove compatible with bash

[Bart Schaefer]

On Jul 1,  7:15pm, Peter Stephenson wrote:
} Subject: Re: PATCH Re: let unset array element remove compatible with bash
}
} On Sun, 01 Jul 2012 09:53:54 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > So the patch below makes
} > 
} >     noglob unset foo[x]
} > 
} > work properly.  If x is an existing index (or valid slice), it is cut
} > from the array as if by foo[x]=().
} 
} Do you mean that?  That's not how bash works, it doesn't cut the
} element, just remove the value at that index

I do mean that; this is the closest zsh can get to the bash behavior, I
think.  Bash represents arrays as linked lists of position-value pairs,
so it's possible for it to have an array with a hole, but zsh uses real
arrays.

It'd be easy enough to tweak the patch to put an empty string into the
element instead of splicing the array, but then it'd be inconsistent with
bash's expansions of ${foo[@]}, ${#foo[@]}, etc.

I suppose we could invent a new metacharacter like Nularg that means the
embedded-in-a-string equivalent of PM_UNSET, but that seems a bit beyond
the scope of the discussion so far.

Or we could change zsh's internal representation of "ordinary" arrays to
use hash tables.  That might not even be too difficult as we already have
code to produce array representations of the keys or values of hashes,
we'd just have to impose an ordering.

I don't think that's necessary in order to fulfill the spirit of this
particular request, though.  Daniel Lin cc'd for comment in case he's not
subscribed to zsh-workers.

Re: PATCH Re: let unset array element remove compatible with bash

[Peter Stephenson]

On Sun, 1 Jul 2012 15:23:10 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jul 1,  7:15pm, Peter Stephenson wrote:
> } Subject: Re: PATCH Re: let unset array element remove compatible with bash
> }
> } On Sun, 01 Jul 2012 09:53:54 -0700
> } Bart Schaefer <schaefer@brasslantern.com> wrote:
> } > So the patch below makes
> } > 
> } >     noglob unset foo[x]
> } > 
> } > work properly.  If x is an existing index (or valid slice), it is cut
> } > from the array as if by foo[x]=().
> } 
> } Do you mean that?  That's not how bash works, it doesn't cut the
> } element, just remove the value at that index
> 
> I do mean that; this is the closest zsh can get to the bash behavior, I
> think.  Bash represents arrays as linked lists of position-value pairs,
> so it's possible for it to have an array with a hole, but zsh uses real
> arrays.
> 
> It'd be easy enough to tweak the patch to put an empty string into the
> element instead of splicing the array, but then it'd be inconsistent with
> bash's expansions of ${foo[@]}, ${#foo[@]}, etc.

Hmm... I'd have said putting in an empty element in was the least
surprising thing to do, at least in the short term.  I don't think
shifting an array so that the numbering is different mimics the ability
to have only certain elements set to any great degree, there are too
many other cases.  If we ever did add the ability, the effect would be
to remove elements that currently appear as empty strings --- so having
the array index shifted would then look wrong.

It's a bit hard to give a definitive answer because, with the
current underlying behaviour, neither meaning of unsetting an array
element actually adds the ability to do something you can't already do.

-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

Re: PATCH Re: let unset array element remove compatible with bash

[Bart Schaefer]

On Jul 2, 10:11am, Peter Stephenson wrote:
}
} Hmm... I'd have said putting in an empty element in was the least
} surprising thing to do, at least in the short term.  I don't think
} shifting an array so that the numbering is different mimics the ability
} to have only certain elements set to any great degree, there are too
} many other cases.

OK, here's one more patch for this, which replaces both 30552 and 30557.

Note that unsetting a slice of a scalar still deletes the slice, but it
might in fact be possible to poke a Nularg in there instead and really
have a 4-character string with a length of 5.  On the other hand, it
does not do sensible things to put a string-consisting-of-Nularg into
an unset array element, because this causes the empty element to be
retained when expanding ${array[*]} whereas the desired effect is to
have an unset element disappear.

I waffled a long time on the "invalid element" error, which in this
incarnation only happens if you try to unset a slice of a numeric type.
If it'd be better that the error become a silent status of 1 (as I had
it in 30557), just put a "-" on the beginning of that line in place of
the space there, and the patch will still apply.


Index: Src/builtin.c
--- ../zsh-forge/current/Src/builtin.c	2012-06-30 10:33:44.000000000 -0700
+++ Src/builtin.c	2012-07-07 09:35:19.000000000 -0700
@@ -3055,8 +3055,35 @@
 		    *sse = ']';
 		}
 		paramtab = tht;
+	    } else if (PM_TYPE(pm->node.flags) == PM_SCALAR ||
+		       PM_TYPE(pm->node.flags) == PM_ARRAY) {
+		struct value vbuf;
+		vbuf.isarr = (PM_TYPE(pm->node.flags) == PM_ARRAY ?
+			      SCANPM_ARRONLY : 0);
+		vbuf.pm = pm;
+		vbuf.flags = 0;
+		vbuf.start = 0;
+		vbuf.end = -1;
+		vbuf.arr = 0;
+		*ss = '[';
+		if (getindex(&ss, &vbuf, SCANPM_ASSIGNING) == 0 &&
+		    vbuf.pm && !(vbuf.pm->node.flags & PM_UNSET)) {
+		    if (PM_TYPE(pm->node.flags) == PM_SCALAR) {
+			setstrvalue(&vbuf, ztrdup(""));
+		    } else {
+			/* start is after the element for reverse index */
+			int start = vbuf.start - !!(vbuf.flags & VALFLAG_INV);
+			if (start < arrlen(vbuf.pm->u.arr)) {
+			    char *arr[2];
+			    arr[0] = "";
+			    arr[1] = 0;
+			    setarrvalue(&vbuf, zarrdup(arr));
+			}
+		    }
+		}
+		returnval = errflag;
+		errflag = 0;
 	    } else {
 		zerrnam(name, "%s: invalid element for unset", s);
 		returnval = 1;
 	    }
 	} else {

Re: PATCH Re: let unset array element remove compatible with bash

[Peter Stephenson]

On Sat, 07 Jul 2012 10:02:03 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jul 2, 10:11am, Peter Stephenson wrote:
> }
> } Hmm... I'd have said putting in an empty element in was the least
> } surprising thing to do, at least in the short term.  I don't think
> } shifting an array so that the numbering is different mimics the ability
> } to have only certain elements set to any great degree, there are too
> } many other cases.
> 
> OK, here's one more patch for this, which replaces both 30552 and 30557.
> 
> Note that unsetting a slice of a scalar still deletes the slice, but it
> might in fact be possible to poke a Nularg in there instead and really
> have a 4-character string with a length of 5.  On the other hand, it
> does not do sensible things to put a string-consisting-of-Nularg into
> an unset array element, because this causes the empty element to be
> retained when expanding ${array[*]} whereas the desired effect is to
> have an unset element disappear.

Thanks, shall we go with what you've got for now and think about extensions
later?

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: PATCH Re: let unset array element remove compatible with bash

[Bart Schaefer]

On Jul 1,  7:15pm, Peter Stephenson wrote:
} Subject: Re: PATCH Re: let unset array element remove compatible with bash
}
} On Sun, 01 Jul 2012 09:53:54 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > So the patch below makes
} > 
} >     noglob unset foo[x]
} > 
} > work properly.  If x is an existing index (or valid slice), it is cut
} > from the array as if by foo[x]=().
} 
} Do you mean that?

For reference, the original question about this was users/16778, in
which Daniel wrote:

>  $unset var[2] ###### BASH only delete one element
>  $var[2]=() ###### ZSH only delete one element

so I made unset var[2] work like var[2]=().  On the other hand he also
wrote:

>   $echo "${#var[@]} ${#var[1]} ${#var[2]} ${#var[3]}"
>   2 4 0 14

so he is expecting var[2] to become an empty element that is not counted
as part of the array, which zsh has no way to represent internally.

Re: PATCH Re: let unset array element remove compatible with bash

[Bart Schaefer]

On Jul 1,  9:53am, Bart Schaefer wrote:
}
} One thing I'm still scratching my head about:  In foo[(i)x] for scalar
} foo, setstrvalue() doesn't properly apply the subscript range returned
} from getindex().  That's why the PM_SCALAR branch below uses getvalue()

I'm not sure what I was doing wrong before, but the below seems to be the
right way to remove the getvalue() in the scalar case.  It doesn't really
save processing because getindex() calls getarg() which eventually calls
getstrvalue() in order to scan th string, but it feels cleaner.

This patch REPLACES the one from 30552, it does not apply on top of it.

Index: Src/builtin.c
--- ../zsh-forge/current/Src/builtin.c	2012-06-30 10:33:44.000000000 -0700
+++ Src/builtin.c	2012-07-02 00:31:05.000000000 -0700
@@ -3055,9 +3055,35 @@
 		    *sse = ']';
 		}
 		paramtab = tht;
+	    } else if (PM_TYPE(pm->node.flags) == PM_SCALAR ||
+		       PM_TYPE(pm->node.flags) == PM_ARRAY) {
+		struct value vbuf;
+		vbuf.isarr = (PM_TYPE(pm->node.flags) == PM_ARRAY ?
+			      SCANPM_ARRONLY : 0);
+		vbuf.pm = pm;
+		vbuf.flags = 0;
+		vbuf.start = 0;
+		vbuf.end = -1;
+		vbuf.arr = 0;
+		*ss = '[';
+		if (getindex(&ss, &vbuf, SCANPM_ASSIGNING) == 0 &&
+		    vbuf.pm && !(vbuf.pm->node.flags & PM_UNSET)) {
+		    if (PM_TYPE(pm->node.flags) == PM_SCALAR) {
+			setstrvalue(&vbuf, ztrdup(""));
+		    } else {
+			/* start is after the element for reverse index */
+			int start = vbuf.start - !!(vbuf.flags & VALFLAG_INV);
+			if (start < arrlen(vbuf.pm->u.arr)) {
+			    char *arr[1];
+			    arr[0] = 0;
+			    setarrvalue(&vbuf, zarrdup(arr));
+			}
+		    }
+		}
+		returnval = errflag;
+		errflag = 0;
 	    } else {
-		zerrnam(name, "%s: invalid element for unset", s);
-		returnval = 1;
+	      returnval = 1;
 	    }
 	} else {
 	    if (unsetparam_pm(pm, 0, 1))

Re: let unset array element remove compatible with bash

[Peter Stephenson]

On Wed, 22 Feb 2012 09:28:27 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
>..
> ... convoluted ...
>...
>      * ... don't quite understand ...
>...
> ... horror ...
>...
> Gaah.

I'm guessing there's no point waiting to get this into 4.3.17?  We've
lived without it for now.  If I get a chance I'll make it this evening,
but this week has been busy.

-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

Re: let unset array element remove compatible with bash

[Bart Schaefer]

On Feb 23, 10:58am, Peter Stephenson wrote:
}
} I'm guessing there's no point waiting to get this into 4.3.17?

No, I should think not.

Also the first hunk of 30261 (suppressing WC_SUBLIST_NOT when errflag)
makes me rather nervous, even though "make check" still passes.  So I
will leave it up to you to commit that if you feel it's safe to include
in 4.3.17, and otherwise I'll commit it sometime after.