Re: [v4l-dvb-maintainer] [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2

[Mike Isely]

On Tue, 2 Sep 2008, Stefan Richter wrote:

> 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?

One of the points behind a good coding style is that it should encourage 
code that is robust against trivial mistakes.  Prefering

	if (a) {
		b;
	}

over

	if (a)
		b;

I consider to be an example of this kind of simple safety.  (And I have 
in the past seen people getting burned from the obvious error of 
sticking a debug printf in between.)  ACTUALLY, I'd much, much rather 
prefer

	if (a) b;

however checkpatch.pl gets angry about that as well (even though the 
kernel CodingStyle document would seem to actually allow this - it's 
still one statement and since "b" is outside the normal flow then it's 
"something to hide" and should be ok in any case).


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

Tell that to those who would use checkpatch.pl to gate incoming 
changesets.


> 
> > 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".

The v4l-maintainer has repeatedly told me otherwise.  His policy is 
basically that it must be checkpatch-clean or it isn't accepted (or at 
least an argument ensues).  He's probably not the only one in the 
community doing this.  Maybe he's getting pushed from above.  I wouldn't 
know.  What I do know is what it does to any subjective reason here.

I agree with your point, and I have raised this exact point when 
checkpatch.pl first got inflicted on me.  The issues I had in fact were 
places where CodingStyle (AFAICT) says it's ok while checkpatch.pl 
complains.  You know what answer I got?  (It wasn't from the v4l-dvb 
maintainer, by the way.)  It was effectively this: "CodingStyle is not 
relevant.  checkpatch.pl is the final authority.  This is what everyone 
does now.  Go away and come back when you have a real point to make."

I happen to have no real problem with CodingStyle.  I think it is well 
thought out and has evolved well over time.  But checkpatch.pl behaves 
like a baseball bat, compared to the fine scalpel that is CodingStyle.  
The checkpatch script has no concept of subjective judgement as you 
point out here.  I have a very big problem with using an imperfect tool 
such as that in a "perfect" no exceptions role as gatekeeper for code 
submissions.  From where I'm sitting - behind such a gate - checkpatch 
has effectively subverted CodingStyle.


> 
> 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.

Amen.


> 
> > 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.

I agree.  I really disliked adding those, and I would rather they not be 
present.  But I have been reminded time and time again that the code had 
to pass checkpatch.pl before it would be pulled.  That led to silliness 
such as this.  I will gladly remove such junk if the maintainer would 
apply a little more subjective reason to his use of checkpatch.pl.

   [...]

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

<RANT>

  <mercifully deleted>

</RANT>

  -Mike


-- 

Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/