try(X) in exec.c ?

try(X) in exec.c ?

[Daniel X. Pape]

I wanted to ask if someone could tell me what the purpose of the return
value of the macro try(X) in exec. is? (In the zsh-3.1.5-pws-4 source)

As far as I can tell, the newly allocated string that try(X) returns is
not assigned to anything at the four places in exec.c that it is used ...
so that string never gets freed.


Thanks - 

Dan

Re: try(X) in exec.c ?

[Bart Schaefer]

On Jan 15, 12:21pm, Daniel X. Pape wrote:
} Subject: try(X) in exec.c ?
}
} I wanted to ask if someone could tell me what the purpose of the return
} value of the macro try(X) in exec. is? (In the zsh-3.1.5-pws-4 source)
} 
} As far as I can tell, the newly allocated string that try(X) returns is
} not assigned to anything at the four places in exec.c that it is used ...
} so that string never gets freed.

It's a macro.  A return statement in a macro doesn't return anything from
the macro, it returns it from the function that calls the macro.  So the
value is returned from findcmd(), and you have to look at the calls to
findcmd() to see whether the string gets freed.

I don't know who named the try() macro; it was a pretty awful choice, and
conventionally any macro that refers to its argument more than once should
be written in all upper case (to remind programmers not to call it with
autoincremented parameters).  Renaming try(X) to MAYBE_RETURN(X) or some
such might be a good idea.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

PATCH: 3.1.5-pws-4: findcmd()

[Peter Stephenson]

"Bart Schaefer" wrote:
> On Jan 15, 12:21pm, Daniel X. Pape wrote:
> } I wanted to ask if someone could tell me what the purpose of the return
> } value of the macro try(X) in exec. is? (In the zsh-3.1.5-pws-4 source)
> 
> It's a macro.  A return statement in a macro doesn't return anything from
> the macro, it returns it from the function that calls the macro.  So the
> value is returned from findcmd(), and you have to look at the calls to
> findcmd() to see whether the string gets freed.

There were one or two leaks.  Looking at where findcmd() is called, it
seems a better bet to have it return something off the heap.  The only
problem is in the completion code, where there could be lots of
different matches adding to the heap, so I added a second argument
which lets findcmd() effectively return a truth value.

I just discovered by looking at the code that `=foo' will expand
aliases if it doesn't find a command.  You learn something new every
day.

*** Src/Zle/zle_tricky.c.findcmd	Mon Jan 18 12:10:23 1999
--- Src/Zle/zle_tricky.c	Mon Jan 18 13:54:25 1999
***************
*** 618,633 ****
  			lst = COMP_EXPAND;
  		    else {
  			int t0, n = 0;
- 			char *fc;
  			struct hashnode *hn;
  
  			for (t0 = cmdnamtab->hsize - 1; t0 >= 0; t0--)
  			    for (hn = cmdnamtab->nodes[t0]; hn;
  				 hn = hn->next) {
! 				if (strpfx(q, hn->nam) && (fc = findcmd(hn->nam))) {
! 				    zsfree(fc);
  				    n++;
- 				}
  				if (n == 2)
  				    break;
  			    }
--- 618,630 ----
  			lst = COMP_EXPAND;
  		    else {
  			int t0, n = 0;
  			struct hashnode *hn;
  
  			for (t0 = cmdnamtab->hsize - 1; t0 >= 0; t0--)
  			    for (hn = cmdnamtab->nodes[t0]; hn;
  				 hn = hn->next) {
! 				if (strpfx(q, hn->nam) && findcmd(hn->nam, 0))
  				    n++;
  				if (n == 2)
  				    break;
  			    }
***************
*** 2355,2361 ****
  {
      int test = 0, sl = strlen(s), pl = rpl, cc = 0, isf = 0;
      int mpl = 0, msl = 0, bpl = brpl, bsl = brsl;
!     char *e = NULL, *tt, *te, *fc, *ms = NULL;
      Comp cp = patcomp;
      HashNode hn;
      Param pm;
--- 2352,2358 ----
  {
      int test = 0, sl = strlen(s), pl = rpl, cc = 0, isf = 0;
      int mpl = 0, msl = 0, bpl = brpl, bsl = brsl;
!     char *e = NULL, *tt, *te, *ms = NULL;
      Comp cp = patcomp;
      HashNode hn;
      Param pm;
***************
*** 2431,2441 ****
  	    }
  	}
  	if (test) {
! 	    fc = NULL;
! 	    if (addwhat == -7 && !(fc = findcmd(s)))
  		return;
- 	    if (fc)
- 		zsfree(fc);
  	    isf = CMF_FILE;
  
  	    if (addwhat == CC_FILES || addwhat == -6 ||
--- 2428,2435 ----
  	    }
  	}
  	if (test) {
! 	    if (addwhat == -7 && !findcmd(s, 0))
  		return;
  	    isf = CMF_FILE;
  
  	    if (addwhat == CC_FILES || addwhat == -6 ||
***************
*** 3165,3171 ****
      /* If the command string starts with `=', try the path name of the *
       * command. */
      if (cmdstr && cmdstr[0] == Equals) {
! 	char *c = findcmd(cmdstr + 1);
  
  	if (c) {
  	    zsfree(cmdstr);
--- 3159,3165 ----
      /* If the command string starts with `=', try the path name of the *
       * command. */
      if (cmdstr && cmdstr[0] == Equals) {
! 	char *c = findcmd(cmdstr + 1, 1);
  
  	if (c) {
  	    zsfree(cmdstr);
***************
*** 3197,3203 ****
  {
      Patcomp pc;
      Comp pat;
!     char *s = findcmd(cmdstr);
  
      for (pc = patcomps; pc; pc = pc->next) {
  	if ((pat = parsereg(pc->pat)) &&
--- 3191,3197 ----
  {
      Patcomp pc;
      Comp pat;
!     char *s = findcmd(cmdstr, 1);
  
      for (pc = patcomps; pc; pc = pc->next) {
  	if ((pat = parsereg(pc->pat)) &&
***************
*** 5681,5687 ****
  	feep();
  	return;
      }
!     str = findcmd(s);
      zsfree(s);
      if (!str) {
  	feep();
--- 5675,5681 ----
  	feep();
  	return;
      }
!     str = findcmd(s, 1);
      zsfree(s);
      if (!str) {
  	feep();
***************
*** 5696,5702 ****
  	cs += cmdwe - cmdwb + strlen(str);
      if (cs > ll)
  	cs = ll;
-     zsfree(str);
  }
  
  /* Extra function added by AR Iano-Fletcher. */
--- 5690,5695 ----
*** Src/builtin.c.findcmd	Mon Jan 18 12:10:23 1999
--- Src/builtin.c	Mon Jan 18 12:28:26 1999
***************
*** 2007,2013 ****
  		puts(wd ? ": none" : " not found");
  		returnval = 1;
  	    }
! 	} else if ((cnam = findcmd(*argv))) {
  	    /* Found external command. */
  	    if (wd) {
  		printf("%s: command\n", *argv);
--- 2007,2013 ----
  		puts(wd ? ": none" : " not found");
  		returnval = 1;
  	    }
! 	} else if ((cnam = findcmd(*argv, 1))) {
  	    /* Found external command. */
  	    if (wd) {
  		printf("%s: command\n", *argv);
***************
*** 2019,2025 ****
  		    print_if_link(cnam);
  		fputc('\n', stdout);
  	    }
- 	    zsfree(cnam);
  	} else {
  	    /* Not found at all. */
  	    if (v || csh || wd)
--- 2019,2024 ----
*** Src/exec.c.findcmd	Mon Jan 18 12:10:23 1999
--- Src/exec.c	Mon Jan 18 12:28:20 1999
***************
*** 457,469 ****
      _exit(1);
  }
  
! #define try(X) { if (iscom(X)) return ztrdup(X); }
  
! /* get the full pathname of an external command */
  
  /**/
  char *
! findcmd(char *arg0)
  {
      char **pp;
      char *z, *s, buf[MAXCMDLEN];
--- 457,473 ----
      _exit(1);
  }
  
! #define RET_IF_COM(X) { if (iscom(X)) return docopy ? dupstring(X) : arg0; }
  
! /*
!  * Get the full pathname of an external command.
!  * If the first argument is zero, just return a non-null pointer if found;
!  * if non-zero, get the actual name using heap memory.
!  */
  
  /**/
  char *
! findcmd(char *arg0, int docopy)
  {
      char **pp;
      char *z, *s, buf[MAXCMDLEN];
***************
*** 476,482 ****
  	return NULL;
      for (s = arg0; *s; s++)
  	if (*s == '/') {
! 	    try(arg0);
  	    if (arg0 == s || unset(PATHDIRS)) {
  		return NULL;
  	    }
--- 480,486 ----
  	return NULL;
      for (s = arg0; *s; s++)
  	if (*s == '/') {
! 	    RET_IF_COM(arg0);
  	    if (arg0 == s || unset(PATHDIRS)) {
  		return NULL;
  	    }
***************
*** 496,508 ****
  			*z++ = '/';
  		    }
  		    strcpy(z, arg0);
! 		    try(buf);
  		}
  	    strcpy(nn, cn->u.name ? *(cn->u.name) : "");
  	    strcat(nn, "/");
  	    strcat(nn, cn->nam);
  	}
! 	try(nn);
      }
      for (pp = path; *pp; pp++) {
  	z = buf;
--- 500,512 ----
  			*z++ = '/';
  		    }
  		    strcpy(z, arg0);
! 		    RET_IF_COM(buf);
  		}
  	    strcpy(nn, cn->u.name ? *(cn->u.name) : "");
  	    strcat(nn, "/");
  	    strcat(nn, cn->nam);
  	}
! 	RET_IF_COM(nn);
      }
      for (pp = path; *pp; pp++) {
  	z = buf;
***************
*** 511,517 ****
  	    *z++ = '/';
  	}
  	strcpy(z, arg0);
! 	try(buf);
      }
      return NULL;
  }
--- 515,521 ----
  	    *z++ = '/';
  	}
  	strcpy(z, arg0);
! 	RET_IF_COM(buf);
      }
      return NULL;
  }
*** Src/subst.c.findcmd	Mon Jan 18 12:10:23 1999
--- Src/subst.c	Mon Jan 18 13:46:21 1999
***************
*** 387,397 ****
  	for (pp = str + 1; !isend2(*pp); pp++);
  	sav = *pp;
  	*pp = 0;
! 	if (!(cnam = findcmd(str + 1))) {
  	    Alias a = (Alias) aliastab->getnode(aliastab, str + 1);
  	    
  	    if (a)
! 		cnam = ztrdup(a->text);
  	    else {
  		if (isset(NOMATCH))
  		    zerr("%s not found", str + 1, 0);
--- 387,397 ----
  	for (pp = str + 1; !isend2(*pp); pp++);
  	sav = *pp;
  	*pp = 0;
! 	if (!(cnam = findcmd(str + 1, 1))) {
  	    Alias a = (Alias) aliastab->getnode(aliastab, str + 1);
  	    
  	    if (a)
! 		cnam = a->text;
  	    else {
  		if (isset(NOMATCH))
  		    zerr("%s not found", str + 1, 0);
***************
*** 399,405 ****
  	    }
  	}
  	*namptr = dupstring(cnam);
- 	zsfree(cnam);
  	if (sav) {
  	    *pp = sav;
  	    *namptr = dyncat(*namptr, pp);
--- 399,404 ----

-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 050 844536
WWW:  http://www.ifh.de/~pws/
Dipartimento di Fisica, Via Buonarroti 2, 56127 Pisa, Italy

Re: PATCH: 3.1.5-pws-4: findcmd()

[Bart Schaefer]

On Jan 18,  2:09pm, Peter Stephenson wrote:
} Subject: PATCH: 3.1.5-pws-4: findcmd()
}
} I added a second argument
} which lets findcmd() effectively return a truth value.

I think it would be worth "documenting" that findcmd() returns its first
argument when the docopy parameter is zero.  Besides, your comment has a
typo.

Index: Src/exec.c
===================================================================
--- exec.c	1999/01/18 16:39:07	1.16
+++ exec.c	1999/01/18 16:42:40
@@ -461,8 +461,8 @@
 
 /*
  * Get the full pathname of an external command.
- * If the first argument is zero, just return a non-null pointer if found;
- * if non-zero, get the actual name using heap memory.
+ * If the second argument is zero, return the first argument if found;
+ * if non-zero, return the path using heap memory.  (RET_IF_COM(X), above).
  */
 
 /**/


-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com