Message-ID: <48BD4DCE.9020507@s5r6.in-berlin.de>
Date:	Tue, 02 Sep 2008 16:29:34 +0200
From:	Stefan Richter <stefanr@...6.in-berlin.de>
To:	Michael Krufky <mkrufky@...uxtv.org>
CC:	Mauro Carvalho Chehab <mchehab@...radead.org>,
	Mike Isely <isely@...ox.com>,
	v4l-dvb maintainer list <v4l-dvb-maintainer@...uxtv.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [v4l-dvb-maintainer] [PULL]	http://linuxtv.org/hg/~mcisely/pvrusb2

Michael Krufky wrote:
> Mike Isely wrote:
>> On Tue, 2 Sep 2008, Mauro Carvalho Chehab wrote:
>>> Please: don't do tricks like this to cheat with checkpatch.pl. The error is
>>> there to point to a Coding Style violation.
>>>
>>> +	if (ret < 0) {
>>> +		/* Keep checkpatch.pl quiet */
>>> +		return ret;
>>> +	}
[...]
>> Forcing this style:
>>
>> 	if (a)
>> 		b;
>>
>> As opposed to the much safer
>>
>> 	if (a) {
>> 		b;
>> 	}
>>
>> is a huge mistake.  Both generate the same code; the second form is 
>> robust against someone later inserting a printk
[...]

If you need this kind of safety measures against errors in future code
changes, could it be that you have some general QA problems?

(However, why waste time arguing over braces or not?)

> I understand that kernel codingstyle forbids single line bracketing,

CodingStyle currently says that braces are not to be used there, *but*
it does not give any explanation for it (other than hinting that the
braces are unnecessary).

It is important to remember that many rules in CodingStyle are _not_
hard rules but just widely (though not universally) accepted
conventions.  And more importantly, checkpatch is even less
authoritative than CodingStyle.  It only gives hints and
recommendations, even if it reports an "error".

If a driver author/maintainer has been using
	if (a) {
		b;
	}
consistently in his driver all the time, why not leave it this way?  It
arguably does not hurt readability.

> but
> codingstyle does not forbid adding comments anywhere in the c source.

Reread the section on commenting.  One very important rule in the Linux
kernel coding style is that we comment sparingly.  We comment with the
goal to keep code readable.

This /* I'll trick checkpatch */ comment is only distracting the reader.
It serves no purpose whatsoever, except to manipulate the output of some
random code submission checking tool.

> Mike added a comment, to create a compromise between kernel codingstyle
> and his own.

The coding style does not contain a rule that says "you may use braces
around single statement blocks if you add a silly, useless, distracting
comment as compensation, because coding style is all about what
checkpatch reports, not about writing well maintainable code".

> This code comes from Mike's svn repository, where he uses
> #ifdefs and various other compat code within his own build environment
> to stay compatible with the v4l-dvb hg tree and the upstream kernel alike.
> 
> Mike is using brackets to ensure that all builds work properly, and to
> ensure that there is no breakage when creating patches for mercurial, or
> when building directly from his svn repository.

Doesn't matter for a mainline release branch.

> There is nothing wrong with the comments that Mike has in his code --
> you should not hold up his merge request for that reason.

There /is/ something wrong with the comment, see above.

> Not only is this another example of checkpatch.pl thwarting development
[...]

With this I agree.
-- 
Stefan Richter
-=====-==--- =--= ---=-
http://arcgraph.de/sr/
--
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/

Powered by blists - more mailing lists