PATCH: Remove some unused assignments/checks noticed by clang

PATCH: Remove some unused assignments/checks noticed by clang

[Mikael Magnusson]

Only two changes result in different code for me, curiously both of the
ones in exec.c, even though they are trivially useless, while none of
the other changes have any effect... That's gcc for you.

Changes that might look strange on first inspection are the removals
of the null checks in compresult.c, note that the first line after each
loop does the same dereference anyway.

There is a big thing removed in zle_tricky.c, yes, that really doesn't
do anything.
---
 Src/Modules/socket.c |    4 ++--
 Src/Modules/zftp.c   |    2 +-
 Src/Zle/compcore.c   |    6 ++----
 Src/Zle/compctl.c    |    4 ++--
 Src/Zle/complist.c   |    4 ++--
 Src/Zle/compmatch.c  |    6 +++---
 Src/Zle/compresult.c |    4 ++--
 Src/Zle/computil.c   |    5 ++---
 Src/Zle/zle_keymap.c |    1 -
 Src/Zle/zle_tricky.c |   13 ++++---------
 Src/exec.c           |    3 ---
 Src/math.c           |    2 --
 Src/module.c         |    3 +--
 Src/sort.c           |    5 ++---
 Src/utils.c          |    6 +++---
 15 files changed, 26 insertions(+), 42 deletions(-)

diff --git a/Src/Modules/socket.c b/Src/Modules/socket.c
index 6c70d31..7369c80 100644
--- a/Src/Modules/socket.c
+++ b/Src/Modules/socket.c
@@ -56,7 +56,7 @@
 static int
 bin_zsocket(char *nam, char **args, Options ops, UNUSED(int func))
 {
-    int err=1, verbose=0, test=0, targetfd=0;
+    int verbose=0, test=0, targetfd=0;
     ZSOCKLEN_T len;
     struct sockaddr_un soun;
     int sfd;
@@ -230,7 +230,7 @@ bin_zsocket(char *nam, char **args, Options ops, UNUSED(int func))
 	soun.sun_family = AF_UNIX;
 	strncpy(soun.sun_path, args[0], sizeof(soun.sun_path)-1);
 	
-	if ((err = connect(sfd, (struct sockaddr *)&soun, sizeof(struct sockaddr_un)))) {
+	if (connect(sfd, (struct sockaddr *)&soun, sizeof(struct sockaddr_un))) {
 	    zwarnnam(nam, "connection failed: %e", errno);
 	    close(sfd);
 	    return 1;
diff --git a/Src/Modules/zftp.c b/Src/Modules/zftp.c
index 1558d35..5d93203 100644
--- a/Src/Modules/zftp.c
+++ b/Src/Modules/zftp.c
@@ -2044,7 +2044,7 @@ zfgetinfo(char *prompt, int noecho)
     }
 
     if (fgets(instr, 256, stdin) == NULL)
-	instr[len = 0] = '\0';
+	instr[0] = '\0';
     else if (instr[len = strlen(instr)-1] == '\n')
 	instr[len] = '\0';
 
diff --git a/Src/Zle/compcore.c b/Src/Zle/compcore.c
index fa8b8c1..da27457 100644
--- a/Src/Zle/compcore.c
+++ b/Src/Zle/compcore.c
@@ -1162,7 +1162,6 @@ check_param(char *s, int set, int test)
 	if (*b == '#' || *b == Pound || *b == '+')
 	    b++;
 
-	e = b;
 	if (br) {
 	    while (*e == (test ? Dnull : '"'))
 		e++, parq++;
@@ -1642,7 +1641,7 @@ set_comp_sep(void)
 	return 1;
     owb = offs;
     offs = soffs;
-    if ((p = check_param(ns, 0, 1))) {
+    if (check_param(ns, 0, 1)) {
 	for (p = ns; *p; p++)
 	    if (*p == Dnull)
 		*p = '"';
@@ -3271,7 +3270,7 @@ dupmatch(Cmatch m, int nbeg, int nend)
 mod_export int
 permmatches(int last)
 {
-    Cmgroup g = amatches, n, opm;
+    Cmgroup g = amatches, n;
     Cmatch *p, *q;
     Cexpl *ep, *eq, e, o;
     LinkList mlist;
@@ -3285,7 +3284,6 @@ permmatches(int last)
     }
     newmatches = fi = 0;
 
-    opm = pmatches;
     pmatches = lmatches = NULL;
     nmatches = smatches = diffmatches = 0;
 
diff --git a/Src/Zle/compctl.c b/Src/Zle/compctl.c
index 873d928..1b87dad0 100644
--- a/Src/Zle/compctl.c
+++ b/Src/Zle/compctl.c
@@ -1466,7 +1466,7 @@ printcompctl(char *s, Compctl cc, int printflags, int ispat)
 	    c = cc2->cond;
 
 	    printf(" '");
-	    for (c = cc2->cond; c;) {
+	    while (c) {
 		/* loop over or's */
 		o = c->or;
 		while (c) {
@@ -2855,7 +2855,7 @@ sep_comp_string(char *ss, char *s, int noffs)
 	return 1;
     owb = offs;
     offs = soffs;
-    if ((p = check_param(ns, 0, 1))) {
+    if (check_param(ns, 0, 1)) {
 	for (p = ns; *p; p++)
 	    if (*p == Dnull)
 		*p = '"';
diff --git a/Src/Zle/complist.c b/Src/Zle/complist.c
index 6d0da44..e87a0ee 100644
--- a/Src/Zle/complist.c
+++ b/Src/Zle/complist.c
@@ -644,7 +644,7 @@ doiscol(int pos)
 static int
 clprintfmt(char *p, int ml)
 {
-    int cc = 0, i = 0, ask, beg;
+    int cc = 0, i = 0, ask;
 
     initiscol();
 
@@ -664,7 +664,7 @@ clprintfmt(char *p, int ml)
 	    putc(*p ^ 32, shout);
 	} else
 	    putc(*p, shout);
-	if ((beg = !(cc % zterm_columns)))
+	if (!(cc % zterm_columns))
 	    ml++;
 	if (mscroll && !(cc % zterm_columns) &&
 	    !--mrestlines && (ask = asklistscroll(ml)))
diff --git a/Src/Zle/compmatch.c b/Src/Zle/compmatch.c
index 4cd3b9f..dcb07f9 100644
--- a/Src/Zle/compmatch.c
+++ b/Src/Zle/compmatch.c
@@ -189,7 +189,7 @@ free_cline(Cline l)
 Cline
 cp_cline(Cline l, int deep)
 {
-    Cline r = NULL, *p = &r, t, lp = NULL;
+    Cline r = NULL, *p = &r, t;
 
     while (l) {
 	if ((t = freecl))
@@ -203,7 +203,7 @@ cp_cline(Cline l, int deep)
 	    if (t->suffix)
 		t->suffix = cp_cline(t->suffix, 0);
 	}
-	*p = lp = t;
+	*p = t;
 	p = &(t->next);
 	l = l->next;
     }
@@ -2022,7 +2022,7 @@ join_strs(int la, char *sa, int lb, char *sb)
 	    lb--;
 	}
     }
-    if (la || lb)
+    if (la || lb || !rp)
 	return NULL;
 
     *rp = '\0';
diff --git a/Src/Zle/compresult.c b/Src/Zle/compresult.c
index f2729a0..641104d 100644
--- a/Src/Zle/compresult.c
+++ b/Src/Zle/compresult.c
@@ -905,7 +905,7 @@ do_allmatches(UNUSED(int end))
     menuacc = 0;
 
     for (minfo.group = amatches;
-	 minfo.group && !(minfo.group)->mcount;
+	 !(minfo.group)->mcount;
 	 minfo.group = (minfo.group)->next);
 
     mc = (minfo.group)->matches;
@@ -2144,7 +2144,7 @@ bld_all_str(Cmatch all)
 
     buf[0] = '\0';
 
-    for (g = amatches; g && !g->mcount; g = g->next);
+    for (g = amatches; !g->mcount; g = g->next);
 
     mp = g->matches;
     while (1) {
diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index e5a9948..aedff2b 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -4465,7 +4465,7 @@ cfp_opt_pats(char **pats, char *matcher)
 	    q = dupstring(q);
 	    t = q + strlen(q) - 1;
 	    if (*t == ')') {
-		for (s = t--; t > q; t--)
+		while (--t)
 		    if (*t == ')' || *t == '|' || *t == '~' || *t == '(')
 			break;
 		if (t != q && *t == '(')
@@ -4479,10 +4479,9 @@ cfp_opt_pats(char **pats, char *matcher)
 		    for (s = add; *s && !idigit(*s); s++);
 		    *s = '\0';
 		} else if (*q == '[') {
-		    int not;
 		    char *x = ++q;
 
-		    if ((not = (*x == '!' || *x == '^')))
+		    if (*x == '!' || *x == '^')
 			x++;
 		    for (; *x; x++) {
 			if (x[1] == '-' && x[2]) {
diff --git a/Src/Zle/zle_keymap.c b/Src/Zle/zle_keymap.c
index a08caa0..8a8a06c 100644
--- a/Src/Zle/zle_keymap.c
+++ b/Src/Zle/zle_keymap.c
@@ -1525,7 +1525,6 @@ getkeycmd(void)
 
 	if (++hops == 20) {
 	    zerr("string inserting another one too many times");
-	    hops = 0;
 	    return NULL;
 	}
 	pb = unmetafy(ztrdup(str), &len);
diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index 7b9ffe9..3b438ec 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -529,7 +529,7 @@ parambeg(char *s)
 	 * or $'...').
 	 */
 	char *b = p + 1, *e = b;
-	int n = 0, br = 1, nest = 0;
+	int n = 0, br = 1;
 
 	if (*b == Inbrace) {
 	    char *tb = b;
@@ -541,10 +541,6 @@ parambeg(char *s)
 	    /* Ignore the possible (...) flags. */
 	    b++, br++;
 	    n = skipparens(Inpar, Outpar, &b);
-
-	    for (tb = p - 1; tb > s && *tb != Outbrace && *tb != Inbrace; tb--);
-	    if (tb > s && *tb == Inbrace && (tb[-1] == String || *tb == Qstring))
-		nest = 1;
 	}
 
 	/* Ignore the stuff before the parameter name. */
@@ -556,7 +552,6 @@ parambeg(char *s)
 	if (*b == '#' || *b == Pound || *b == '+')
 	    b++;
 
-	e = b;
 	if (br) {
 	    while (*e == Dnull)
 		e++;
@@ -1540,7 +1535,7 @@ get_comp_string(void)
 	     * so start at the beginning of the line and continue
 	     * until cspos.
 	     */
-	    wptr = cptr = zlemetaline;
+	    wptr = zlemetaline;
 	    for (;;) {
 		cptr = itype_end(wptr, IIDENT, 0);
 		if (cptr == wptr) {
@@ -1563,7 +1558,7 @@ get_comp_string(void)
 	    char *sqbr = zlemetaline + wb - 1, *cptr, *wptr;
 
 	    /* Need to search forward for word characters */
-	    cptr = wptr = zlemetaline;
+	    wptr = zlemetaline;
 	    for (;;) {
 		cptr = itype_end(wptr, IIDENT, 0);
 		if (cptr == wptr) {
@@ -1591,7 +1586,7 @@ get_comp_string(void)
     }
     /* This variable will hold the current word in quoted form. */
     offs = zlemetacs - wb;
-    if ((p = parambeg(s))) {
+    if (parambeg(s)) {
 	for (p = s; *p; p++)
 	    if (*p == Dnull)
 		*p = '"';
diff --git a/Src/exec.c b/Src/exec.c
index 3526f83..8fd32d9 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2272,9 +2272,7 @@ execsubst(LinkList strs)
     if (strs) {
 	prefork(strs, esprefork);
 	if (esglob) {
-	    LinkList ostrs = strs;
 	    globlist(strs, 0);
-	    strs = ostrs;
 	}
     }
 }
@@ -4131,7 +4129,6 @@ execfuncdef(Estate state, UNUSED(int do_exec))
 
     end = beg + WC_FUNCDEF_SKIP(state->pc[-1]);
     names = ecgetlist(state, *state->pc++, EC_DUPTOK, &htok);
-    nprg = end - beg;
     sbeg = *state->pc++;
     nstrs = *state->pc++;
     npats = *state->pc++;
diff --git a/Src/math.c b/Src/math.c
index caff06d..35b362d 100644
--- a/Src/math.c
+++ b/Src/math.c
@@ -969,7 +969,6 @@ void
 op(int what)
 {
     mnumber a, b, c, *spval;
-    char *lv;
     int tp = type[what];
 
     if (errflag)
@@ -1155,7 +1154,6 @@ op(int what)
 	}
 	if (tp & (OP_E2|OP_E2IO)) {
 	    struct mathvalue *mvp = stack + sp + 1;
-	    lv = stack[sp+1].lval;
 	    push(setmathvar(mvp,c), mvp->lval, 0);
 	} else
 	    push(c,NULL, 0);
diff --git a/Src/module.c b/Src/module.c
index 219bdfa..26eb779 100644
--- a/Src/module.c
+++ b/Src/module.c
@@ -426,14 +426,13 @@ static int
 add_autobin(const char *module, const char *bnam, int flags)
 {
     Builtin bn;
-    int ret;
 
     bn = zshcalloc(sizeof(*bn));
     bn->node.nam = ztrdup(bnam);
     bn->optstr = ztrdup(module);
     if (flags & FEAT_AUTOALL)
 	bn->node.flags |= BINF_AUTOALL;
-    if ((ret = addbuiltin(bn))) {
+    if (addbuiltin(bn)) {
 	builtintab->freenode(&bn->node);
 	if (!(flags & FEAT_IGNORE))
 	    return 1;
diff --git a/Src/sort.c b/Src/sort.c
index 3d00bb5..e20e386 100644
--- a/Src/sort.c
+++ b/Src/sort.c
@@ -220,7 +220,7 @@ strmetasort(char **array, int sortwhat, int *unmetalenp)
     for (arrptr = array, sortptrarrptr = sortptrarr, sortarrptr = sortarr;
 	 *arrptr; arrptr++, sortptrarrptr++, sortarrptr++) {
 	char *metaptr;
-	int needlen, needalloc;
+	int needlen;
 	*sortptrarrptr = sortarrptr;
 	sortarrptr->orig = *arrptr;
 
@@ -251,8 +251,7 @@ strmetasort(char **array, int sortwhat, int *unmetalenp)
 	 * Either we're going to need to copy it to transform it,
 	 * or we need to unmetafy it.
 	 */
-	if ((needalloc = (sortwhat &
-			  (SORTIT_IGNORING_CASE|SORTIT_IGNORING_BACKSLASHES)))
+	if ((sortwhat & (SORTIT_IGNORING_CASE|SORTIT_IGNORING_BACKSLASHES))
 	    || *metaptr == Meta) {
 	    char *s, *t, *src = *arrptr, *dst;
 	    int len;
diff --git a/Src/utils.c b/Src/utils.c
index 6fdf369..3e0bcc8 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -2000,7 +2000,7 @@ arrlen(char **s)
     return count;
 }
 
-/* Skip over a balanced pair of parenthesis. */
+/* Skip over a balanced pair of parentheses. */
 
 /**/
 mod_export int
@@ -4693,7 +4693,7 @@ addunprintable(char *v, const char *u, const char *uend)
 mod_export char *
 quotestring(const char *s, char **e, int instring)
 {
-    const char *u, *tt;
+    const char *u;
     char *v;
     int alloclen;
     char *buf;
@@ -4744,7 +4744,7 @@ quotestring(const char *s, char **e, int instring)
 	break;
     }
 
-    tt = quotestart = v = buf = zshcalloc(alloclen);
+    quotestart = v = buf = zshcalloc(alloclen);
 
     DPUTS(instring < QT_BACKSLASH || instring == QT_BACKTICK ||
 	  instring > QT_SINGLE_OPTIONAL,
-- 
1.7.4-rc1

Re: PATCH: Remove some unused assignments/checks noticed by clang

[Wayne Davison]

On Fri, May 13, 2011 at 5:23 PM, Mikael Magnusson <mikachu@gmail.com> wrote:
> --- a/Src/Zle/compcore.c
> +++ b/Src/Zle/compcore.c
> @@ -1162,7 +1162,6 @@ check_param(char *s, int set, int test)
>        if (*b == '#' || *b == Pound || *b == '+')
>            b++;
>
> -       e = b;
>        if (br) {
>            while (*e == (test ? Dnull : '"'))
>                e++, parq++;

That change looks wrong to me.  You should remove the " = b"
initializer in the declaration:

        char *b = p + 1, *e = b, *ie;

The later assignment could occur after b changes value, and thus
removing it means that e starts further back in the string.

The same comment applies to the similar change in zle_tricky.c.

> --- a/Src/Zle/computil.c
> +++ b/Src/Zle/computil.c
> @@ -4465,7 +4465,7 @@ cfp_opt_pats(char **pats, char *matcher)
>            q = dupstring(q);
>            t = q + strlen(q) - 1;
>            if (*t == ')') {
> -               for (s = t--; t > q; t--)
> +               while (--t)
>                    if (*t == ')' || *t == '|' || *t == '~' || *t == '(')
>                        break;
>                if (t != q && *t == '(')

That while should be while (--t > q), since indexing a string back to
0 is not a good idea.

..wayne..

Re: PATCH: Remove some unused assignments/checks noticed by clang

[Mikael Magnusson]

On 14 May 2011 19:22, Wayne Davison <wayned@users.sourceforge.net> wrote:
> On Fri, May 13, 2011 at 5:23 PM, Mikael Magnusson <mikachu@gmail.com> wrote:
>> --- a/Src/Zle/compcore.c
>> +++ b/Src/Zle/compcore.c
>> @@ -1162,7 +1162,6 @@ check_param(char *s, int set, int test)
>>        if (*b == '#' || *b == Pound || *b == '+')
>>            b++;
>>
>> -       e = b;
>>        if (br) {
>>            while (*e == (test ? Dnull : '"'))
>>                e++, parq++;
>
> That change looks wrong to me.  You should remove the " = b"
> initializer in the declaration:
>
>        char *b = p + 1, *e = b, *ie;
>
> The later assignment could occur after b changes value, and thus
> removing it means that e starts further back in the string.

Oops, but also weird. With the change, the md5sum of Src/zsh remains
the same, but Src/Zle/compcore..o changes.

> The same comment applies to the similar change in zle_tricky.c.
>
>> --- a/Src/Zle/computil.c
>> +++ b/Src/Zle/computil.c
>> @@ -4465,7 +4465,7 @@ cfp_opt_pats(char **pats, char *matcher)
>>            q = dupstring(q);
>>            t = q + strlen(q) - 1;
>>            if (*t == ')') {
>> -               for (s = t--; t > q; t--)
>> +               while (--t)
>>                    if (*t == ')' || *t == '|' || *t == '~' || *t == '(')
>>                        break;
>>                if (t != q && *t == '(')
>
> That while should be while (--t > q), since indexing a string back to
> 0 is not a good idea.

Indeed. I'd better look at the whole patch once more. :)

-- 
Mikael Magnusson

Re: PATCH: Remove some unused assignments/checks noticed by clang

[Mikael Magnusson]

On 14 May 2011 02:23, Mikael Magnusson <mikachu@gmail.com> wrote:
> Only two changes result in different code for me, curiously both of the
> ones in exec.c, even though they are trivially useless, while none of
> the other changes have any effect... That's gcc for you.
>
> Changes that might look strange on first inspection are the removals
> of the null checks in compresult.c, note that the first line after each
> loop does the same dereference anyway.
>
> There is a big thing removed in zle_tricky.c, yes, that really doesn't
> do anything.

> --- a/Src/module.c
> +++ b/Src/module.c
> @@ -426,14 +426,13 @@ static int
>  add_autobin(const char *module, const char *bnam, int flags)
>  {
>     Builtin bn;
> -    int ret;
>
>     bn = zshcalloc(sizeof(*bn));
>     bn->node.nam = ztrdup(bnam);
>     bn->optstr = ztrdup(module);
>     if (flags & FEAT_AUTOALL)
>        bn->node.flags |= BINF_AUTOALL;
> -    if ((ret = addbuiltin(bn))) {
> +    if (addbuiltin(bn)) {
>        builtintab->freenode(&bn->node);
>        if (!(flags & FEAT_IGNORE))
>            return 1;

Should this slightly out of view "return 0;" perhaps change to "return
ret;" instead?

-- 
Mikael Magnusson

Re: PATCH: Remove some unused assignments/checks noticed by clang

[Bart Schaefer]

On May 14,  7:43pm, Mikael Magnusson wrote:
}
} Oops, but also weird. With the change, the md5sum of Src/zsh remains
} the same, but Src/Zle/compcore..o changes.

You need to either static-link everything, or also check the md5sum
of all the loadable modules separately.