Re: [PATCH] typeset reply array

Re: [PATCH] typeset reply array

[Clint Adams]

I think that some of these will break things. For example, if you keep 
_comp_dumpfile local to compinit, when/if compinit calls compdump,
problems might ensue.

I don't know why we can't call compdump with an argument though.

On Sat, May 17, 2008 at 12:55:24PM +0100, martin f. krafft wrote:
> From: martin f. krafft <madduck@madduck.net>
> 
> With warn_create_global set, the function otherwise causes warnings
> like this:
> 
>   url-quote-magic:1: array parameter reply created globally in
>   function
> 
> Signed-off-by: martin f. krafft <madduck@debian.org>
> ---
>  Functions/Zle/url-quote-magic |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/Functions/Zle/url-quote-magic b/Functions/Zle/url-quote-magic
> index 826d26d..e15d117 100644
> --- a/Functions/Zle/url-quote-magic
> +++ b/Functions/Zle/url-quote-magic
> @@ -65,11 +65,12 @@ zstyle -m ':url-quote-magic:\*' url-metas '*' ||
>      zstyle ':url-quote-magic:*' url-metas '*?[]^(|)~#{}='
>  
>  zstyle -m ':url-quote-magic:\*' url-seps '*' ||
> -    zstyle -e ':url-quote-magic:*' url-seps 'reply=(";&<>${histchars[1]}")'
> +    zstyle -e ':url-quote-magic:*' url-seps 'typeset -a reply; reply=(";&<>${histchars[1]}")'
>  
>  zstyle -m :url-quote-magic url-globbers '*' ||
>      zstyle -e :url-quote-magic url-globbers \
>  	'zmodload -i zsh/parameter;
> +	 typeset -a reply;
>  	 reply=( noglob
>  		 ${(k)galiases[(R)(* |)(noglob|urlglobber|globurl) *]:-}
>  		 ${(k)aliases[(R)(* |)(noglob|urlglobber|globurl) *]:-} )'
> -- 
> 1.5.5.1

On Sat, May 17, 2008 at 01:11:17PM +0100, martin f. krafft wrote:
> With warn_create_global set, the function otherwise causes warnings
> like this:
> 
> compinit:123: scalar parameter _comp_dumpfile created globally in function
> compinit:130: array parameter _comp_options created globally in function
> compinit:164: scalar parameter _comp_setup created globally in function
> compinit:169: array parameter compprefuncs created globally in function
> compinit:170: array parameter comppostfuncs created globally in function
> 
> Signed-off-by: martin f. krafft <madduck@debian.org>
> ---
>  Completion/compinit |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/Completion/compinit b/Completion/compinit
> index a4edd24..30dc26c 100644
> --- a/Completion/compinit
> +++ b/Completion/compinit
> @@ -120,13 +120,14 @@ typeset -gA _lastcomp
>  # Remember dumpfile.
>  if [[ -n $_i_dumpfile ]]; then
>    # Explicitly supplied dumpfile.
> -  _comp_dumpfile="$_i_dumpfile"
> +  local _comp_dumpfile="$_i_dumpfile"
>  else
>    _comp_dumpfile="${ZDOTDIR:-$HOME}/.zcompdump"
>  fi
>  
>  # The standard options set in completion functions.
>  
> +typeset -a _comp_options
>  _comp_options=(
>         bareglobqual
>         extendedglob
> @@ -157,6 +158,7 @@ _comp_options=(
>  # have a valid stdin descriptor (zle closes it before calling widgets)
>  # and don't get confused by user's ZERR trap handlers.
>  
> +typeset -l _comp_setup
>  _comp_setup='setopt localoptions localtraps ${_comp_options[@]};
>               local IFS=$'\'\ \\t\\r\\n\\0\''
>               exec </dev/null;
> @@ -165,8 +167,8 @@ _comp_setup='setopt localoptions localtraps ${_comp_options[@]};
>  # These can hold names of functions that are to be called before/after all
>  # matches have been generated.
>  
> -compprefuncs=()
> -comppostfuncs=()
> +typeset -a compprefuncs; compprefuncs=()
> +typeset -a comppostfuncs; comppostfuncs=()
>  
>  # Loading it now ensures that the `funcstack' parameter is always correct.
>  
> -- 
> 1.5.5.1

Re: [PATCH] typeset reply array

[Bart Schaefer]

On May 18,  5:33am, Clint Adams wrote:
}
} I think that some of these will break things.

(Stylistically, "local" or "declare" should probably be used instead of
"typeset" when the intent is to make local parameters.)

The right thing to do (if there is a right thing) is use "typeset -g ..."
to explicitly create them as globals where appropriate.  This suppresses
the warning without changing the behavior.

On the other hand I think zsh could make reply/REPLY into particular
exceptions to the warning.  Although they're not "special parameters"
in the true sense, it's also the case that they're never (or always,
depending on your point of view) intended to be local to the scope
where they're assigned.  It's not safe to use them for anything but
their reserved purpose because they'll get cleared in the internals
if any operation that may employ them is called.

-- 

Re: [PATCH] typeset reply array

[martin f krafft]

[-- Attachment #1: Type: text/plain, Size: 686 bytes --]

also sprach Bart Schaefer <schaefer@brasslantern.com> [2008.05.18.1740 +0100]:
> On the other hand I think zsh could make reply/REPLY into
> particular exceptions to the warning. 

I would agree with that, along with many more of the parameters
listed in "PARAMETERS USED BY THE SHELL" ... the warning also
happens for HISTFILE and REPORTTIME, for instance.

-- 
 .''`.   martin f. krafft <madduck@debian.org>
: :'  :  proud Debian developer, author, administrator, and user
`. `'`   http://people.debian.org/~madduck - http://debiansystem.info
  `-  Debian - when you have better things to do than fixing systems
 
i welcome your constructive criticism and corrections.

[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/) --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] typeset reply array

[Peter Stephenson]

On Sun, 18 May 2008 05:33:01 +0000
Clint Adams <schizo@debian.org> wrote:
> I think that some of these will break things. For example, if you keep 
> _comp_dumpfile local to compinit, when/if compinit calls compdump,
> problems might ensue.

Commonly used but non-global parameters such as "reply" could, and
probably should, be made local to the top-level of completion at around
the point where the options get set.  This would save them appearing in
the user's own space, which was the whole point of warncreateglobal in
the first place.

I don't think any special *internal* handling is warranted for
parameters that aren't themselves special to the shell.

A "typeset -g" for variables that *should* be global makes perfect
sense.  The "-g" should definitely be present even if the code isn't
inside a function (although the compinit stuff is), since we shouldn't
make that assumption.

> I don't know why we can't call compdump with an argument though.

I can't see why we shouldn't either.  I have a vague memory of wondering
that before.

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

Re: [PATCH] typeset reply array

[Peter Stephenson]

On Sun, 18 May 2008 18:17:35 +0100
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> Commonly used but non-global parameters such as "reply" could, and
> probably should, be made local to the top-level of completion at around
> the point where the options get set.  This would save them appearing in
> the user's own space, which was the whole point of warncreateglobal in
> the first place.
> 
> A "typeset -g" for variables that *should* be global makes perfect
> sense.  The "-g" should definitely be present even if the code isn't
> inside a function (although the compinit stuff is), since we shouldn't
> make that assumption.

This does that.  It's the same basic principle as the original patch,
but should be safe.

Index: Completion/compinit
===================================================================
RCS file: /cvsroot/zsh/zsh/Completion/compinit,v
retrieving revision 1.20
diff -u -r1.20 compinit
--- Completion/compinit	12 Apr 2008 16:32:43 -0000	1.20
+++ Completion/compinit	18 May 2008 19:23:01 -0000
@@ -118,6 +118,7 @@
 typeset -gA _lastcomp
 
 # Remember dumpfile.
+typeset -g _comp_dumpfile
 if [[ -n $_i_dumpfile ]]; then
   # Explicitly supplied dumpfile.
   _comp_dumpfile="$_i_dumpfile"
@@ -127,6 +128,7 @@
 
 # The standard options set in completion functions.
 
+typeset -ga _comp_options
 _comp_options=(
        bareglobqual
        extendedglob
@@ -158,14 +160,18 @@
 # have a valid stdin descriptor (zle closes it before calling widgets)
 # and don't get confused by user's ZERR trap handlers.
 
+typeset -g _comp_setup
 _comp_setup='setopt localoptions localtraps ${_comp_options[@]};
              local IFS=$'\'\ \\t\\r\\n\\0\''
              exec </dev/null;
-             trap - ZERR'
+             trap - ZERR
+             local -a reply
+             local REPLY'
 
 # These can hold names of functions that are to be called before/after all
 # matches have been generated.
 
+typeset -ga compprefuncs comppostfuncs
 compprefuncs=()
 comppostfuncs=()
 
 
-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/