From: Hugh Sasse <hgs@...>
Date: 2006-08-22T01:36:43+09:00
Subject: Building with -Wall => questions

Having built the 1.8.5-preview4 ruby on Solaris9, I wondered if
there was anything I could contribute before the final release.
I've been reading Code Quality by Spinellis
http://www.spinellis.gr/codequality/
and saw the remarks about turning warnings on to find more edge cases.
So I just tried changing CFLAGS in the Makefile to include -Wall.
The first part of the results is like this:

neelix hgs 12 %> gmake
gcc -Wall -g -O2  -DRUBY_EXPORT  -Wall -I. -I.  -c array.c
gcc -Wall -g -O2  -DRUBY_EXPORT  -Wall -I. -I.  -c bignum.c
bignum.c: In function 'rb_cstr_to_inum':
bignum.c:457: warning: suggest parentheses around assignment used as truth value
gcc -Wall -g -O2  -DRUBY_EXPORT  -Wall -I. -I.  -c class.c
gcc -Wall -g -O2  -DRUBY_EXPORT  -Wall -I. -I.  -c compar.c
gcc -Wall -g -O2  -DRUBY_EXPORT  -Wall -I. -I.  -c dir.c
dir.c: In function 'fnmatch':
dir.c:161: warning: suggest parentheses around assignment used as truth value
dir.c: In function 'glob_helper':
dir.c:982: warning: value computed is not used
dir.c:1009: warning: value computed is not used
dir.c:1025: warning: value computed is not used
dir.c:1053: warning: value computed is not used
dir.c:1100: warning: value computed is not used
gcc -Wall -g -O2  -DRUBY_EXPORT  -Wall -I. -I.  -c dln.c
dln.c: In function 'dln_find_1':


The "suggest parentheses around assignment" isn't really important, it
means 
neelix hgs 51 %> gdiff -pu1 bignum.c.orig bignum.c
--- bignum.c.orig       2006-04-09 16:10:36.000000000 +0000
+++ bignum.c    2006-08-21 15:29:17.189742000 +0000
@@ -456,3 +456,3 @@ rb_cstr_to_inum(str, base, badcheck)
     for (i=len;i--;) zds[i]=0;
-    while (c = *str++) {
+    while ((c = *str++)) {
        if (c == '_') {
neelix hgs 52 %>

In other words, use double parentheses to mark deliberate assignments.
One can debate whether that is right. A possible loss of clarity (extra
parentheses) is traded for a warning when -Wall is in effect.

The warnings for "value computed is not used" are all for 

            sys_warning(path);

and similar uses of sys_warning().  sys_warning is a macro

#define sys_warning(val) \
    ((flags & GLOB_VERBOSE) && rb_protect((VALUE (*)_((VALUE)))sys_warning_1, (VALUE)(val), 0))


Clearly the && is being used as a shortcut operator.  Kernighan and Pike

http://cm.bell-labs.com/cm/cs/tpop/

talks about macros, and generally discourages their use now that
compilers can do inlining.  Since the output of sys_warning is never
used, should this be changed into a void function?

And finally, should I just shut up?  Well, it probably needs asking:
now may well be the wrong time to mention such things so close to
1.8.5; there may be good reasons why macros are preferred over
functions for some platforms.  I can probably create some patches for
many of these cases but if now is not the time, then I'll leave it 
until later.

        Thank you,
        Hugh