Enabling more warnings?

Enabling more warnings?

[Wayne Davison]

What do folks think about turning on more gcc warnings and then changing
the code to eliminate them?  Adding -W to the CFLAGS generates a bunch
of "unused parameter" warnings, which we can either turn off with the
-Wno-unused-parameters option, or tweak in the code explicitly, like the
following:

#define UNUSED(x) x __attribute__((__unused__))
int
bin_set(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))

Or maybe this:

#define _U_ __attribute__((__unused__))
int
bin_set(char *nam, char **args, Options ops _U_, int func _U_)

I personally think that the UNUSED(x) one reads better than _U_, but I'm
not sure we want these sprinkled around in the code (rsync uses this
method, but we don't have nearly as many unused parameters as zsh does).

Thoughts?  The -W option turns up other warnings to fix as well, so I'm
also curious how people react to turning on -W by default.

..wayne..

Re: Enabling more warnings?

[Clint Adams]

> Thoughts?  The -W option turns up other warnings to fix as well, so I'm
> also curious how people react to turning on -W by default.

Personally, I find build systems that override my CFLAGS to be pretty
annoying.

As far as the warning hunt goes, I'll change the Debian builds from
"-Wall -O2" to "-Wall -W -O2".  Once I remember to do that, the build
logs will be accessible from

http://buildd.debian.org/build.php?arch=&pkg=zsh

and

http://buildd.debian.org/build.php?arch=&pkg=zsh-beta


That won't catch most compat functions or ZSH_MEM stuff though.

Re: Enabling more warnings?

[Wayne Davison]

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

Attached is a patch that tweaks the default gcc CFLAGS in configure to
add "-W -Wno-unused-parameter" and then fixes most of the new warnings
that showed up -- mainly signed/unsigned stuff, but some other minor
things too (I didn't try to fix the longjump/vfork-clobber warnings).

..wayne..

[-- Attachment #2: morewarnings.patch --]
[-- Type: text/plain, Size: 12485 bytes --]

--- configure.ac	6 Apr 2004 09:25:17 -0000	1.17
+++ configure.ac	25 May 2004 20:05:52 -0000
@@ -346,13 +346,13 @@ dnl   else use -O
 if test -n "$auto_cflags" && test ."$ansi2knr" != .yes; then
   if test "${enable_zsh_debug}" = yes; then
     if test -n "$GCC"; then
-      CFLAGS="$CFLAGS -Wall -Wmissing-prototypes -ggdb"
+      CFLAGS="$CFLAGS -Wall -W -Wno-unused-parameter -Wmissing-prototypes -ggdb"
     else
       CFLAGS="$CFLAGS -g"
     fi
   else
     if test -n "$GCC"; then
-      CFLAGS="$CFLAGS -Wall -Wmissing-prototypes -O2"
+      CFLAGS="$CFLAGS -Wall -W -Wno-unused-parameter -Wmissing-prototypes -O2"
     else
       CFLAGS="$CFLAGS -O"
     fi
--- Src/glob.c	6 Apr 2004 17:45:47 -0000	1.33
+++ Src/glob.c	25 May 2004 20:05:54 -0000
@@ -2562,7 +2562,7 @@ remnulargs(char *s)
 static int
 qualdev(char *name, struct stat *buf, off_t dv, char *dummy)
 {
-    return buf->st_dev == dv;
+    return (off_t)buf->st_dev == dv;
 }
 
 /* number of hard links to file */
--- Src/hashtable.c	20 Apr 2004 12:11:16 -0000	1.18
+++ Src/hashtable.c	25 May 2004 20:05:55 -0000
@@ -946,7 +946,7 @@ static struct reswd reswds[] = {
     {NULL, "time", 0, TIME},
     {NULL, "until", 0, UNTIL},
     {NULL, "while", 0, WHILE},
-    {NULL, NULL}
+    {NULL, NULL, 0, 0}
 };
 
 /* hash table containing the reserved words */
--- Src/jobs.c	25 May 2004 18:39:11 -0000	1.29
+++ Src/jobs.c	25 May 2004 20:05:56 -0000
@@ -64,12 +64,12 @@ mod_export struct job *jobtab;
 /* Size of the job table. */
 
 /**/
-mod_export size_t jobtabsize;
+mod_export int jobtabsize;
 
 /* The highest numbered job in the jobtable */
 
 /**/
-mod_export size_t maxjob;
+mod_export int maxjob;
 
 /* If we have entered a subshell, the original shell's job table. */
 static struct job *oldjobtab;
@@ -1399,7 +1399,7 @@ expandjobtab(void)
 void
 maybeshrinkjobtab(void)
 {
-    size_t jobbound;
+    int jobbound;
 
     queue_signals();
     jobbound = maxjob + MAXJOBS_ALLOC - (maxjob % MAXJOBS_ALLOC);
--- Src/loop.c	15 Dec 2003 22:45:29 -0000	1.11
+++ Src/loop.c	25 May 2004 20:05:56 -0000
@@ -338,7 +338,8 @@ selectlist(LinkList l, size_t start)
     for (t1 = start; t1 != colsz && t1 - start < lines - 2; t1++) {
 	ap = arr + t1;
 	do {
-	    int t2 = strlen(*ap) + 2, t3;
+	    size_t t2 = strlen(*ap) + 2;
+	    int t3;
 
 	    fprintf(stderr, "%d) %s", t3 = ap - arr + 1, *ap);
 	    while (t3)
--- Src/params.c	21 May 2004 20:05:16 -0000	1.83
+++ Src/params.c	25 May 2004 20:05:58 -0000
@@ -224,7 +224,8 @@ IPDEF8("MODULE_PATH", &module_path, "mod
 #define IPDEF9(A,B,C) IPDEF9F(A,B,C,0)
 IPDEF9F("*", &pparams, NULL, PM_ARRAY|PM_SPECIAL|PM_DONTIMPORT|PM_READONLY),
 IPDEF9F("@", &pparams, NULL, PM_ARRAY|PM_SPECIAL|PM_DONTIMPORT|PM_READONLY),
-{NULL, NULL},
+{NULL,NULL,0,BR(NULL),SFN(NULL),GFN(NULL),NULL,0,NULL,NULL,NULL,0},
+
 #define IPDEF10(A,B,C) {NULL,A,PM_ARRAY|PM_SPECIAL,BR(NULL),SFN(C),GFN(B),stdunsetfn,10,NULL,NULL,NULL,0}
 
 /* The following parameters are not available in sh/ksh compatibility *
@@ -252,7 +253,7 @@ IPDEF9F("path", &path, "PATH", PM_RESTRI
 
 IPDEF10("pipestatus", pipestatgetfn, pipestatsetfn),
 
-{NULL, NULL}
+{NULL,NULL,0,BR(NULL),SFN(NULL),GFN(NULL),NULL,0,NULL,NULL,NULL,0},
 };
 
 /*
@@ -3335,7 +3336,7 @@ findenv(char *name, int *pos)
 
 
     eq = strchr(name, '=');
-    nlen = eq ? eq - name : strlen(name);
+    nlen = eq ? eq - name : (int)strlen(name);
     for (ep = environ; *ep; ep++) 
 	if (!strncmp (*ep, name, nlen) && *((*ep)+nlen) == '=') {
 	    if (pos)
@@ -3709,6 +3710,8 @@ static const struct paramtypes pmtypes[]
     { PM_EXPORTED, "exported", 'x', 0}
 };
 
+#define PMTYPES_SIZE ((int)(sizeof(pmtypes)/sizeof(struct paramtypes)))
+
 /**/
 mod_export void
 printparamnode(HashNode hn, int printflags)
@@ -3727,8 +3730,7 @@ printparamnode(HashNode hn, int printfla
 	int doneminus = 0, i;
 	const struct paramtypes *pmptr;
 
-	for (pmptr = pmtypes, i = 0; i < sizeof(pmtypes)/sizeof(*pmptr);
-	     i++, pmptr++) {
+	for (pmptr = pmtypes, i = 0; i < PMTYPES_SIZE; i++, pmptr++) {
 	    int doprint = 0;
 	    if (pmptr->flags & PMTF_TEST_LEVEL) {
 		if (p->level)
--- Src/parse.c	11 Mar 2004 14:25:12 -0000	1.42
+++ Src/parse.c	25 May 2004 20:06:00 -0000
@@ -2524,9 +2524,8 @@ load_dump_header(char *nam, char *name, 
 	}
 	memcpy(head, buf, (FD_PRELEN + 1) * sizeof(wordcode));
 
-	if (read(fd, head + (FD_PRELEN + 1),
-		 len - ((FD_PRELEN + 1) * sizeof(wordcode))) !=
-	    len - ((FD_PRELEN + 1) * sizeof(wordcode))) {
+	len -= (FD_PRELEN + 1) * sizeof(wordcode);
+	if (read(fd, head + (FD_PRELEN + 1), len) != len) {
 	    close(fd);
 	    zwarnnam(nam, "invalid zwc file: %s" , name, 0);
 	    return NULL;
@@ -3125,7 +3124,7 @@ check_dump_file(char *file, struct stat 
 	    }
 	    d = (Wordcode) zalloc(h->len + po);
 
-	    if (read(fd, ((char *) d) + po, h->len) != h->len) {
+	    if (read(fd, ((char *) d) + po, h->len) != (int)h->len) {
 		close(fd);
 		zfree(d, h->len);
 
--- Src/pattern.c	8 Mar 2004 12:00:15 -0000	1.18
+++ Src/pattern.c	25 May 2004 20:06:02 -0000
@@ -425,7 +425,7 @@ patcompile(char *exp, int inflags, char 
 		    len = 0;
 		    for (; pscan; pscan = PATNEXT(pscan))
 			if (P_OP(pscan) == P_EXACTLY &&
-			    strlen((char *)P_OPERAND(pscan)) >= len) {
+			    (int)strlen((char *)P_OPERAND(pscan)) >= len) {
 			    lng = (char *)P_OPERAND(pscan);
 			    len = strlen(lng);
 			}
@@ -2023,7 +2023,7 @@ patmatch(Upat prog)
 		    int ptlen = strlen(patinput);
 		    int oplen = strlen(nextop);
 		    /* Are we in the right range? */
-		    if (oplen > strlen(min ? METANEXT(start) : start) ||
+		    if (oplen > (int)strlen(min ? METANEXT(start) : start) ||
 			oplen < ptlen)
 			return 0;
 		    /* Yes, just position appropriately and test. */
--- Src/prompt.c	4 May 2004 16:43:43 -0000	1.17
+++ Src/prompt.c	25 May 2004 20:06:02 -0000
@@ -283,11 +283,11 @@ putpromptchar(int doprint, int endchar)
 			test = 1;
 		    break;
 		case '#':
-		    if (geteuid() == arg)
+		    if (geteuid() == (uid_t)arg)
 			test = 1;
 		    break;
 		case 'g':
-		    if (getegid() == arg)
+		    if (getegid() == (gid_t)arg)
 			test = 1;
 		    break;
 		case 'j':
--- Src/subst.c	30 Aug 2003 19:12:18 -0000	1.36
+++ Src/subst.c	25 May 2004 20:06:04 -0000
@@ -1637,7 +1637,7 @@ paramsubst(LinkList l, LinkNode n, char 
 		 * Bet that's easier said than done.
 		 */
 		val = getstrvalue(v);
-		fwidth = v->pm->ct ? v->pm->ct : strlen(val);
+		fwidth = v->pm->ct ? v->pm->ct : (int)strlen(val);
 		switch (v->pm->flags & (PM_LEFT | PM_RIGHT_B | PM_RIGHT_Z)) {
 		    char *t;
 		    unsigned int t0;
--- Src/utils.c	5 May 2004 17:00:58 -0000	1.61
+++ Src/utils.c	25 May 2004 20:06:07 -0000
@@ -515,7 +515,7 @@ finddir(char *s)
     if(!strcmp(s, finddir_full) && *finddir_full)
 	return finddir_last;
 
-    if(strlen(s) >= ffsz) {
+    if ((int)strlen(s) >= ffsz) {
 	free(finddir_full);
 	finddir_full = zalloc(ffsz = strlen(s) * 2);
     }
@@ -1908,7 +1908,7 @@ colonsplit(char *s, int uniq)
 	for (; *t && *t != ':'; t++);
 	if (uniq)
 	    for (p = ret; p < ptr; p++)
-		if (strlen(*p) == t - s && ! strncmp(*p, s, t - s))
+		if ((int)strlen(*p) == t - s && ! strncmp(*p, s, t - s))
 		    goto cont;
 	*ptr = (char *) zalloc((t - s) + 1);
 	ztrncpy(*ptr++, s, t - s);
--- Src/Modules/datetime.c	4 May 2004 16:43:43 -0000	1.8
+++ Src/Modules/datetime.c	25 May 2004 20:06:07 -0000
@@ -48,7 +48,7 @@ bin_strftime(char *nam, char **argv, Opt
     }
 
     secs = (time_t)strtoul(argv[1], &endptr, 10);
-    if (secs == ULONG_MAX) {
+    if (secs == (time_t)ULONG_MAX) {
 	zwarnnam(nam, "%s: %e", argv[1], errno);
 	return 1;
     } else if (*endptr != '\0') {
--- Src/Modules/zftp.c	24 Mar 2004 11:31:33 -0000	1.30
+++ Src/Modules/zftp.c	25 May 2004 20:06:09 -0000
@@ -431,7 +431,7 @@ zfunalarm(void)
 	 * I love the way alarm() uses unsigned int while time_t
 	 * is probably something completely different.
 	 */
-	time_t tdiff = time(NULL) - oaltime;
+	unsigned int tdiff = time(NULL) - oaltime;
 	alarm(oalremain < tdiff ? 1 : oalremain - tdiff);
     } else
 	alarm(0);
--- Src/Zle/compcore.c	29 Oct 2003 19:17:48 -0000	1.65
+++ Src/Zle/compcore.c	25 May 2004 20:06:11 -0000
@@ -1477,7 +1477,7 @@ set_comp_sep(void)
     sl = strlen(s);
     if (swe > sl) {
 	swe = sl;
-	if (strlen(ns) > swe - swb + 1)
+	if ((int)strlen(ns) > swe - swb + 1)
 	    ns[swe - swb + 1] = '\0';
     }
     qs = (issq ? dupstring(s + swe) : rembslash(s + swe));
@@ -1862,8 +1862,8 @@ addmatches(Cadata dat, char **argv)
 	    llpl = strlen(lpre);
 	    llsl = strlen(lsuf);
 
-	    if (llpl + strlen(compqiprefix) + strlen(lipre) != origlpre ||
-		llsl + strlen(compqisuffix) + strlen(lisuf) != origlsuf)
+	    if (llpl + (int)strlen(compqiprefix) + (int)strlen(lipre) != origlpre
+	     || llsl + (int)strlen(compqisuffix) + (int)strlen(lisuf) != origlsuf)
 		lenchanged = 1;
 
 	    /* Test if there is an existing -P prefix. */
--- Src/Zle/compctl.c	15 Feb 2004 12:55:43 -0000	1.17
+++ Src/Zle/compctl.c	25 May 2004 20:06:17 -0000
@@ -2892,7 +2892,7 @@ sep_comp_string(char *ss, char *s, int n
     sl = strlen(s);
     if (swe > sl) {
 	swe = sl;
-	if (strlen(ns) > swe - swb + 1)
+	if ((int)strlen(ns) > swe - swb + 1)
 	    ns[swe - swb + 1] = '\0';
     }
     qs = tricat(multiquote(s + swe, 0), qisuf, "");
--- Src/Zle/complete.c	29 Oct 2003 19:17:48 -0000	1.24
+++ Src/Zle/complete.c	25 May 2004 20:06:17 -0000
@@ -788,7 +788,7 @@ do_comp_vars(int test, int na, char *sa,
 	if (!na)
 	    return 1;
 	if (na > 0 &&
-	    strlen(test == CVT_PRENUM ? compprefix : compsuffix) >= na) {
+	    (int)strlen(test == CVT_PRENUM ? compprefix : compsuffix) >= na) {
 	    if (mod) {
 		if (test == CVT_PRENUM)
 		    ignore_prefix(na);
--- Src/Zle/computil.c	29 Oct 2003 19:17:48 -0000	1.85
+++ Src/Zle/computil.c	25 May 2004 20:06:19 -0000
@@ -690,8 +690,8 @@ cd_get(char **params)
                     strcpy(dbuf, cd_state.sep);
                     memcpy(dbuf + cd_state.slen,
                            str->desc,
-                           (strlen(str->desc) >= dlen ? dlen - 1 :
-                            strlen(str->desc)));
+                           ((int)strlen(str->desc) >= dlen ? dlen - 1 :
+                            (int)strlen(str->desc)));
                     *dp++ = ztrdup(dbuf);
                 }
                 mats[0] = *dp = NULL;
@@ -1640,7 +1640,7 @@ ca_inactive(Cadef d, char **xor, int cur
     if ((xor || opts) && cur <= compcurrent) {
 	Caopt opt;
 	char *x;
-	int sl = (d->set ? strlen(d->set) : -1), set = 0;
+	int sl = (d->set ? (int)strlen(d->set) : -1), set = 0;
 
 	for (; (x = (opts ? "-" : *xor)); xor++) {
             if (optname && optname[0] == x[0] && strcmp(optname, x))
--- Src/Zle/zle_hist.c	8 Mar 2004 11:44:14 -0000	1.11
+++ Src/Zle/zle_hist.c	25 May 2004 20:06:20 -0000
@@ -808,7 +808,7 @@ doisearch(char **args, int dir)
 			else
 			    pos -= 1 + (pos != 1 && s[pos-2] == Meta);
 		    } else if (sbuf[0] != '^') {
-			if (pos >= strlen(s+1))
+			if (pos >= (int)strlen(s+1))
 			    skip_line = 1;
 			else
 			    pos += 1 + (s[pos] == Meta);
--- Src/Zle/zle_refresh.c	29 Oct 2003 19:17:48 -0000	1.10
+++ Src/Zle/zle_refresh.c	25 May 2004 20:06:21 -0000
@@ -1033,7 +1033,7 @@ tc_rightcurs(int ct)
    if we're anywhere in the prompt, goto the left column and write the whole
    prompt out unless ztrlen(lpromptbuf) == lpromptw : we can cheat then */
     if (vln == 0 && i < lpromptw && !(termflags & TERM_SHORT)) {
-	if (strlen(lpromptbuf) == lpromptw)
+	if ((int)strlen(lpromptbuf) == lpromptw)
 	    fputs(lpromptbuf + i, shout);
 	else if (tccan(TCRIGHT) && (tclen[TCRIGHT] * ct <= ztrlen(lpromptbuf)))
 	    /* it is cheaper to send TCRIGHT than reprint the whole prompt */
--- Src/Zle/zle_tricky.c	11 Mar 2004 14:25:14 -0000	1.43
+++ Src/Zle/zle_tricky.c	25 May 2004 20:06:23 -0000
@@ -399,7 +399,7 @@ checkparams(char *p)
 	for (hn = paramtab->nodes[t0]; n < 2 && hn; hn = hn->next)
 	    if (pfxlen(p, hn->nam) == l) {
 		n++;
-		if (strlen(hn->nam) == l)
+		if ((int)strlen(hn->nam) == l)
 		    e = 1;
 	    }
     return (n == 1) ? (getsparam(p) != NULL) :
@@ -1472,7 +1472,7 @@ get_comp_string(void)
 
     if (!isset(IGNOREBRACES)) {
 	/* Try and deal with foo{xxx etc. */
-	char *curs = s + (isset(COMPLETEINWORD) ? offs : strlen(s));
+	char *curs = s + (isset(COMPLETEINWORD) ? offs : (int)strlen(s));
 	char *predup = dupstring(s), *dp = predup;
 	char *bbeg = NULL, *bend = NULL, *dbeg = NULL;
 	char *lastp = NULL, *firsts = NULL;

Re: Enabling more warnings?

[Clint Adams]

> #define UNUSED(x) x __attribute__((__unused__))

I also vote for this instead of -Wno-unused-parameters.

Re: Enabling more warnings?

[Peter Stephenson]

Clint Adams wrote:
> > #define UNUSED(x) x __attribute__((__unused__))
> 
> I also vote for this instead of -Wno-unused-parameters.

Assuming there is an #ifdef __GNUC__ around, of course.

pws


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: Enabling more warnings?

[Wayne Davison]

On Tue, May 25, 2004 at 01:18:49PM -0700, Wayne Davison wrote:
> Attached is a patch that [...] fixes most of the new warnings that
> showed up [with -W -Wno-unused-parameter]

Since no one objected to the warning fixes, I've reviewed them for
correctness and checked them in.  I did not commit any changes to
configure, though.

We had one vote so far for marking all the unused parameters with an
UNUSED() macro, and I'd be willing to do that and check in the changes.
Any objections?

..wayne..