PATCH: minor variable allocation change in add-zsh-hook

PATCH: minor variable allocation change in add-zsh-hook

[Matthew Hamilton]

The 'local usage' variable is allocated in main, outside of the if loop
that determines if it is going to print the usage information. This
means time is spent allocating that variable, when in most cases, it
will never be printed. It would be better to set it within the if loop,
or alternatively, not using a variable and simply output the usage text
as as literal string (as many other functions do).

A trace of the time being needlessly being spent allocating the variable
can be seen here: https://gist.github.com/Eriner/3192c9eb98fabdd70607

It's not that much time, but it adds up and is inefficient/unnecessary.

diff --git a/Functions/Misc/add-zsh-hook b/Functions/Misc/add-zsh-hook
index ee37d67..bccd115 100644
--- a/Functions/Misc/add-zsh-hook
+++ b/Functions/Misc/add-zsh-hook
@@ -19,7 +19,6 @@ hooktypes=(
   chpwd precmd preexec periodic zshaddhistory zshexit
   zsh_directory_name
 )
-local usage="Usage: $0 hook function\nValid hooks are:\n  $hooktypes"

 local opt
 local -a autoopts
@@ -58,6 +57,7 @@ if (( list )); then
   typeset -mp "(${1:-${(@j:|:)hooktypes}})_functions"
   return $?
 elif (( help || $# != 2 || ${hooktypes[(I)$1]} == 0 )); then
+  local usage="Usage: $0 hook function\nValid hooks are:\n  $hooktypes"
   print -u$(( 2 - help )) $usage
   return $(( 1 - help ))
 fi

Re: PATCH: minor variable allocation change in add-zsh-hook

[Mikael Magnusson]

On Fri, Oct 2, 2015 at 11:06 PM, Matthew Hamilton <M@tthamilton.com> wrote:
> The 'local usage' variable is allocated in main, outside of the if loop
> that determines if it is going to print the usage information. This
> means time is spent allocating that variable, when in most cases, it
> will never be printed. It would be better to set it within the if loop,
> or alternatively, not using a variable and simply output the usage text
> as as literal string (as many other functions do).
>
> A trace of the time being needlessly being spent allocating the variable
> can be seen here: https://gist.github.com/Eriner/3192c9eb98fabdd70607
>
> It's not that much time, but it adds up and is inefficient/unnecessary.
>
> diff --git a/Functions/Misc/add-zsh-hook b/Functions/Misc/add-zsh-hook
> index ee37d67..bccd115 100644
> --- a/Functions/Misc/add-zsh-hook
> +++ b/Functions/Misc/add-zsh-hook
> @@ -19,7 +19,6 @@ hooktypes=(
>    chpwd precmd preexec periodic zshaddhistory zshexit
>    zsh_directory_name
>  )
> -local usage="Usage: $0 hook function\nValid hooks are:\n  $hooktypes"
>
>  local opt
>  local -a autoopts
> @@ -58,6 +57,7 @@ if (( list )); then
>    typeset -mp "(${1:-${(@j:|:)hooktypes}})_functions"
>    return $?
>  elif (( help || $# != 2 || ${hooktypes[(I)$1]} == 0 )); then
> +  local usage="Usage: $0 hook function\nValid hooks are:\n  $hooktypes"
>    print -u$(( 2 - help )) $usage
>    return $(( 1 - help ))
>  fi

You would need to add tens of thousands of hooks before this would
make any difference, and then you're already far past the point of
sanity. I like the way the current code separates the content and
logic. There is also no such thing as an "if loop".

-- 
Mikael Magnusson

Re: PATCH: minor variable allocation change in add-zsh-hook

[Mikael Magnusson]

On Fri, Oct 2, 2015 at 11:44 PM, Matthew Hamilton <M@tthamilton.com> wrote:
>
>
> On 10/02/2015 05:38 PM, Mikael Magnusson wrote:
>> On Fri, Oct 2, 2015 at 11:06 PM, Matthew Hamilton <M@tthamilton.com> wrote:
>>> The 'local usage' variable is allocated in main, outside of the if loop
>>> that determines if it is going to print the usage information. This
>>> means time is spent allocating that variable, when in most cases, it
>>> will never be printed. It would be better to set it within the if loop,
>>> or alternatively, not using a variable and simply output the usage text
>>> as as literal string (as many other functions do).
>>>
>>> A trace of the time being needlessly being spent allocating the variable
>>> can be seen here: https://gist.github.com/Eriner/3192c9eb98fabdd70607
>>>
>>> It's not that much time, but it adds up and is inefficient/unnecessary.
>>>
>>> diff --git a/Functions/Misc/add-zsh-hook b/Functions/Misc/add-zsh-hook
>>> index ee37d67..bccd115 100644
>>> --- a/Functions/Misc/add-zsh-hook
>>> +++ b/Functions/Misc/add-zsh-hook
>>> @@ -19,7 +19,6 @@ hooktypes=(
>>>    chpwd precmd preexec periodic zshaddhistory zshexit
>>>    zsh_directory_name
>>>  )
>>> -local usage="Usage: $0 hook function\nValid hooks are:\n  $hooktypes"
>>>
>>>  local opt
>>>  local -a autoopts
>>> @@ -58,6 +57,7 @@ if (( list )); then
>>>    typeset -mp "(${1:-${(@j:|:)hooktypes}})_functions"
>>>    return $?
>>>  elif (( help || $# != 2 || ${hooktypes[(I)$1]} == 0 )); then
>>> +  local usage="Usage: $0 hook function\nValid hooks are:\n  $hooktypes"
>>>    print -u$(( 2 - help )) $usage
>>>    return $(( 1 - help ))
>>>  fi
>>
>> You would need to add tens of thousands of hooks before this would
>> make any difference, and then you're already far past the point of
>> sanity. I like the way the current code separates the content and
>> logic. There is also no such thing as an "if loop".
>>
>
> You're correct, I meant 'if statement'. My counter to that point would
> be to match how many of the other functions do it; they simply print the
> help and do not allocate any variables.
>
> ex: Functions/TCP/tcp_shoot
>
> if [[ $# -ne 2 ]]; then
>     print "Usage: tcp_dump host port

This one was a good example, because the usage message is actually
incorrect (tcp_dump instead of tcp_shoot) :).

> ex: Functions/VCS_Info/vcs_info_hookdel
>
> if (( ${#argv} < 2 )); then
>     print 'usage: vcs_info_hookdel [-a] <HOOK> <FUNCTION(s)...>'

Well, it's true that add-zsh-hook is the only function that does the
'local usage' thing, and I don't feel strongly about it. It just looks
nice to me the way it is in that particular case, what with the long
case statement coming before the print and all.

-- 
Mikael Magnusson