linux-kernel - Re: [PATCH] Staging: comedi: serial2002: fixed consistent spacing issue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 20 Mar 2013 15:29:51 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Dan Carpenter <dan.carpenter@...cle.com>
Cc:	Alice Ferrazzi <alice.ferrazzi@...il.com>,
	gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Staging: comedi: serial2002: fixed consistent spacing
 issue

On Wed, Mar 20, 2013 at 03:47:53PM +0300, Dan Carpenter wrote:

> The original code here needs to broken up into functions so it isn't
> squashed up against the 80 character limit.

I'd say what needs to be done to the original code...

Observation 1:
if (foo) {
	A /* two lines */
} else {
	B /* huge pile of shite */
}
return result;

is equivalent to

if (foo) {
	A
	return result;
}
B
return result;

Observation 2:
while (1) {
	A	/* a couple of lines */
	if (foo) {
		break;
	} else {
		B /* huge pile of shite */
	}
}
is equivalent to
while (1) {
	A
	if (foo)
		break;
	B
}

Observation 3:
while (1) {
	A	/* moderate pile of shite, assigning foo */
	if (foo) {
		B /* huge pile of shite */
	}
}
is equivalent to
while (1) {
	A
	if (!foo)
		continue;
	B
}

Observation 4: functions are there for purpose.  When you have two identical
piles of garbage (avert your eyes, or risk taking another look at your dinner)
such as
				int unit, sign, min;
				unit =
				    (data.value >> 10) &
				    0x7;
				sign =
				    (data.value >> 13) &
				    0x1;
				min =
				    (data.value >> 14) &
				    0xfffff;

				switch (unit) {
				case 0:{
						min =
						    min
						    *
						    1000000;
					}
					break;
				case 1:{
						min =
						    min
						    *
						    1000;
					}
					break;
				case 2:{
						min =
						    min
						    * 1;
					}
					break;
				}
				if (sign)
					min = -min;

you just might consider turning that pile of excrements into a helper
function.  Incidentally, min = min * 1 is somewhat, er, pointless...

Observation 5:
for (i = 0; i <= 4; i++) {
{
	switch (i) {
	case 0: c = non_NULL_1; ... break;
	case 1: c = non_NULL_2; ... break;
	case 2: c = non_NULL_3; ... break;
	case 3: c = non_NULL_4; ... break;
	case 4: c = non_NULL_5; ... break;
	default: c = NULL; break;
	}
	if (c) {
		pile_of_shite
	}
}
might, perhaps, be taking defensive programming a bit too far...

Observation 6:
the Vogon whose brain has produced that code up had been brought up on Pascal,
Ada or something worse, and had been badly traumatized by semantics of switch
and break.
switch (foo) {
	case 0: {
		bar = baz;
	} break;
	case 1: {
	.....
}
is not quite conventional for C.

Observation 7: down, not across...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/