linux-kernel - Re: [PATCH 109/148] include/asm-x86/serial.h: checkpatch cleanups

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]

Message-ID: <20080325133810.GA10044@elte.hu>
Date:	Tue, 25 Mar 2008 14:38:10 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Jörn Engel <joern@...fs.org>
Cc:	David Miller <davem@...emloft.net>, jirislaby@...il.com,
	viro@...IV.linux.org.uk, joe@...ches.com, tglx@...utronix.de,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 109/148] include/asm-x86/serial.h: checkpatch cleanups
	- formatting only

* Jörn Engel <joern@...fs.org> wrote:

> > The current visual inconsistency between subsystems makes the Linux 
> > kernel appear rather unpleasant and unprofessional to new kernel 
> > developers. This is not just embarrasing to us (we want to write the 
> > best OS on the planet), it is also actively harmful: such a 
> > "consistent style does not matter" stance turns away people who have 
> > taste and tends to attract people who have no taste - which i'm sure 
> > you'll agree with results in a deadly spiral if it gets strong 
> > enough.
> 
> I disagree with that assertion.  My favorite example of where
> CodingStyle has gone too far is this:
> 	for (i=0; i<10; i++)
> While the official document demands four extra spaces, I _hate_ them. 
> Whitespace offers visual grouping.  The lack of whitespace around the 
> binary operators emphasizes that one kind of grouping is stonger than 
> another.  Ever since this binary operator testament was added to our 
> Holy Canon, I started violating the coding style on purpose.  Imo this 
> is beyond silly.

you picked an borderline case without showing the full effects of your 
choice of style - but still even in this example you are wrong i 
believe. Look at how inconsistent this looks:

 	for (i=0; i<10; i++) {
		l = 10;
		if (k <= 10)
			k = 11;
	}

(the inconsistent 'i=0' versus 'l = 10')

so in your style we'd have to write it as:

 	for (i=0; i<10; i++) {
		l=10;
		if (k<=10)
			k=11;
	}

which, on one hand, looks unprofessional (in fixed width font), but on 
the other hand, the literals and operators are way too "close" to each 
other and operators are easily missed and mis-identified visually - 
causing bugs.

For example the 'k' and the '<=' operator may look visually similar and 
can be "blended", making it easy to skip over 'k=10' versus 'k<=10' - 
while 'k = 10' clearly stands apart from 'k <= 10'. [and syntax 
highlighting does not help with this particular problem]

in Documentation/CodingStyle it looks:

 	for (i = 0; i < 10; i++) {
		l = 10;
		if (k <= 10)
			k = 11;
	}

which is certainly reasonable and groups safely.

yes - you can have arguments one way or another, but there's nothing 
worse than maintainers each going towards their own _arbitrary_ and 
often clearly inferior coding style, which is inconsistent within the 
same file.

I at least make the point that i'm trying to converge to 
Documentation/CodingStyle. Is it arbitrary? Yes, to a fair degree, but 
it certainly conveys a very strong, unambiguous sense of taste.

	Ingo
--
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/