* PATCH: minor variable allocation change in add-zsh-hook @ 2015-10-02 21:06 Matthew Hamilton 2015-10-02 21:38 ` Mikael Magnusson 0 siblings, 1 reply; 3+ messages in thread From: Matthew Hamilton @ 2015-10-02 21:06 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: PATCH: minor variable allocation change in add-zsh-hook 2015-10-02 21:06 PATCH: minor variable allocation change in add-zsh-hook Matthew Hamilton @ 2015-10-02 21:38 ` Mikael Magnusson [not found] ` <560EFAC3.2050806@tthamilton.com> 0 siblings, 1 reply; 3+ messages in thread From: Mikael Magnusson @ 2015-10-02 21:38 UTC (permalink / raw) To: Matthew Hamilton; +Cc: zsh workers 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
[parent not found: <560EFAC3.2050806@tthamilton.com>]
* Re: PATCH: minor variable allocation change in add-zsh-hook [not found] ` <560EFAC3.2050806@tthamilton.com> @ 2015-10-02 22:03 ` Mikael Magnusson 0 siblings, 0 replies; 3+ messages in thread From: Mikael Magnusson @ 2015-10-02 22:03 UTC (permalink / raw) To: Matthew Hamilton, zsh workers 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-10-02 22:03 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-02 21:06 PATCH: minor variable allocation change in add-zsh-hook Matthew Hamilton 2015-10-02 21:38 ` Mikael Magnusson [not found] ` <560EFAC3.2050806@tthamilton.com> 2015-10-02 22:03 ` Mikael Magnusson
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).