Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id ; Wed, 21 Nov 2001 05:40:59 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id ; Wed, 21 Nov 2001 05:40:50 -0500 Received: from [195.66.192.167] ([195.66.192.167]:49938 "EHLO Port.imtp.ilyichevsk.odessa.ua") by vger.kernel.org with ESMTP id ; Wed, 21 Nov 2001 05:40:36 -0500 Content-Type: text/plain; charset="us-ascii" From: vda To: linux-kernel@vger.kernel.org Subject: [BUG] Bad #define, nonportable C, missing {} Date: Wed, 21 Nov 2001 12:40:17 +0000 X-Mailer: KMail [version 1.2] MIME-Version: 1.0 Message-Id: <01112112401703.01961@nemo> Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Hi, Upon random browsing in the kernel tree I noticed in accel.c: *a++ = byte_rev[*a] which isn't 100% correct C AFAIK. At least Stroustrup in his C++ book warns that this kind of code has to be avoided. Wrote a script to catch similar things all over the tree (attached). Found some buglets. Here they are: drivers/message/i2o/i2o_config.c:#define MODINC(x,y) (x = x++ % y) --------------------------------------------------- Bad code style. Bad name (sounds like 'module inc'). I can't even tell from this define what the hell it is trying to do: x++ will return unchanged x, then we obtain (x mod y), then we store it into x... and why x++ then??! Alan, seems like you can help here... drivers/isdn/isdn_audio.c: *buff++ = table[*(unsigned char *)buff]; drivers/video/riva/accel.c: *a++ = byte_rev[*a]; drivers/video/riva/accel.c:/* *a++ = byte_rev[*a]; drivers/video/riva/accel.c: *a++ = byte_rev[*a];*/ drivers/usb/se401.c: *frame++=(((*frame^255)*(*frame^255))/255)^255; arch/mips/lib/tinycon.c: *(caddr++) = *(caddr + size_x); arch/mips/lib/tinycon.c: *(caddr++) = (*caddr & 0xff00) | (unsigned short) ' '; (btw, tinycon.c seriously needs Lindenting) ------------------------------------------------------------------ Undefined behavior by C std: inc/dec may happen before dereference. Probably GCC is doing inc after right side eval, but standards say nothing about it AFAIK. Move ++ out of the statement to be safe: *a++ = byte_rev[*a]; => *a = byte_rev[*a]; a++; Patch is attached. drivers/block/paride/pf.c: if (l==0x20) j--; targ[j]=0; drivers/block/paride/pg.c: if (l==0x20) j--; targ[j]=0; drivers/block/paride/pt.c: if (l==0x20) j--; targ[j]=0; (these files need Lindenting too) ---------- Missing {} Either a bug or a very bad style (so bad that I can even imagine that it is NOT a bug). Please double check before applying the patch! -- vda ======= bad_c.diff ======= diff -u --recursive linux-2.4.13-old/arch/mips/lib/tinycon.c linux-2.4.13-new/arch/mips/lib/tinycon.c --- linux-2.4.13-old/arch/mips/lib/tinycon.c Thu Jun 26 17:33:37 1997 +++ linux-2.4.13-new/arch/mips/lib/tinycon.c Wed Nov 21 00:54:05 2001 @@ -83,14 +83,18 @@ register int i; caddr = vram_addr; - for(i=0; i/dev/null \ | grep -E '(for)|(Documentation)' -v >!vpp_v 2>&1 & # *var-- ... *var do_grep '[^0-9A-Za-z_]([A-Za-z_][0-9A-Za-z_]*)--.*[^0-9A-Za-z_]\1[^0-9A-Za-z_]' * 2>/dev/null \ | grep -E '(for)|(Documentation)' -v >!vmm_v 2>&1 & # *var ... *var++ do_grep '[^0-9A-Za-z_]([A-Za-z_][0-9A-Za-z_]*)[^0-9A-Za-z_].*[^0-9A-Za-z_]\1\+\+' * 2>/dev/null \ | grep -E '(for)|(Documentation)' -v >!v_vpp 2>&1 & # *var ... *var-- do_grep '[^0-9A-Za-z_]([A-Za-z_][0-9A-Za-z_]*)[^0-9A-Za-z_].*[^0-9A-Za-z_]\1--' * 2>/dev/null \ | grep -E '(for)|(Documentation)' -v >!v_vmm 2>&1 & - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/