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: