Subject: bin/20546: unmaintainable code award for sort(1)
To: None <gnats-bugs@gnats.netbsd.org>
From: seebs <seebs@vash.cel.plethora.net>
List: netbsd-bugs
Date: 03/02/2003 02:20:12
>Number:         20546
>Category:       bin
>Synopsis:       sort(1) uses a very bad macro name
>Confidential:   no
>Severity:       serious
>Priority:       low
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Mar 02 00:21:00 PST 2003
>Closed-Date:
>Last-Modified:
>Originator:     seebs
>Release:        NetBSD 1.6P
>Organization:
>Environment:
System: NetBSD vash.cel.plethora.net 1.6P NetBSD 1.6P (VASH) #1: Fri Feb 28 22:14:13 CST 2003 seebs@vash.cel.plethora.net:/usr/src/sys/arch/i386/compile/VASH i386
Architecture: i386
Machine: i386
>Description:
	There's a macro in sort(1)'s fields.c which is in all lowercase,
	but modifies its argument, and has a name which is at best a bad
	pun.

>How-To-Repeat:
	Read the code.

>Fix:
	This fix changes the name from "blancmange" (a sort of food) into
	SKIP_BLANKS.  The all-caps name helps warn a maintainer that it's
	a macro - and thus that it can modify the value passed to it.  It
	also clarifies what it does.  I think.  I have no idea why it's
	not using "isblank(*++(ptr))", but I'm too afraid to look.

*** fields.c	Wed Dec 25 22:02:56 2002
--- /tmp/fields.c	Sun Mar  2 02:17:14 2003
***************
*** 45,51 ****
  __SCCSID("@(#)fields.c	8.1 (Berkeley) 6/6/93");
  #endif /* not lint */
  
! #define blancmange(ptr) {					\
  	if (BLANK & d_mask[*(ptr)])				\
  		while (BLANK & d_mask[*(++(ptr))]);		\
  }
--- 45,51 ----
  __SCCSID("@(#)fields.c	8.1 (Berkeley) 6/6/93");
  #endif /* not lint */
  
! #define SKIP_BLANKS(ptr) {					\
  	if (BLANK & d_mask[*(ptr)])				\
  		while (BLANK & d_mask[*(++(ptr))]);		\
  }
***************
*** 159,165 ****
  	start = icol.p->start;
  	lineend = clist[ncols].end;
  	if (flags & BI)
! 		blancmange(start);
  	start += icol.indent;
  	start = min(start, lineend);
  
--- 159,165 ----
  	start = icol.p->start;
  	lineend = clist[ncols].end;
  	if (flags & BI)
! 		SKIP_BLANKS(start);
  	start += icol.indent;
  	start = min(start, lineend);
  
***************
*** 169,175 ****
  		if (tcol.indent) {
  			end = tcol.p->start;
  			if (flags & BT)
! 				blancmange(end);
  			end += tcol.indent;
  			end = min(end, lineend);
  		} else
--- 169,175 ----
  		if (tcol.indent) {
  			end = tcol.p->start;
  			if (flags & BT)
! 				SKIP_BLANKS(end);
  			end += tcol.indent;
  			end = min(end, lineend);
  		} else
***************
*** 235,241 ****
  	 *	(-r: +/-)(sign: +/-)(expsign: +/-)
  	 */
  	or_sign = sign ^ expsign ^ Rflag;
! 	blancmange(line);
  	if (*line == '-') {	/* set the sign */
  		or_sign ^= 1;
  		sign = 0;
--- 235,241 ----
  	 *	(-r: +/-)(sign: +/-)(expsign: +/-)
  	 */
  	or_sign = sign ^ expsign ^ Rflag;
! 	SKIP_BLANKS(line);
  	if (*line == '-') {	/* set the sign */
  		or_sign ^= 1;
  		sign = 0;
>Release-Note:
>Audit-Trail:
>Unformatted: