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

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

[Michael Krufky]

Mike Isely wrote:
> On Tue, 2 Sep 2008, Mauro Carvalho Chehab wrote:
>
>   
>> On Sun, 31 Aug 2008 20:20:40 -0500 (CDT)
>> Mike Isely <isely@isely.net> wrote:
>>
>>     
>>> Mauro:
>>>
>>> Please pull from http://linuxtv.org/hg/~mcisely/pvrusb2 for:
>>>
>>> - pvrusb2: Handle USB ID 2040:2950 same as 2040:2900
>>> - pvrusb2: Add comment elaborating on direct use of swab32()
>>> - pvrusb2: Remove BKL
>>> - pvrusb2: Fail gracefully if an alien USB ID is used
>>> - pvrusb2: Implement crop support
>>> - pvrusb2: Mark crop window size change as being disruptive to the encoder
>>> - pvrusb2: Be able to programmatically retrieve a control's default value
>>> - pvrusb2: Implement default value retrieval in sysfs interface
>>> - pvrusb2: Implement cropping pass through
>>> - pvrusb2: Disable virtual IR device when not needed.
>>>
>>>  pvrusb2-ctrl.c           |   10 
>>>  pvrusb2-ctrl.h           |    2 
>>>  pvrusb2-devattr.c        |    2 
>>>  pvrusb2-hdw-internal.h   |   11 -
>>>  pvrusb2-hdw.c            |  475 +++++++++++++++++++++++++++++++++++++++++------
>>>  pvrusb2-hdw.h            |   13 +
>>>  pvrusb2-i2c-chips-v4l2.c |    7 
>>>  pvrusb2-i2c-cmd-v4l2.c   |   86 ++++++--
>>>  pvrusb2-i2c-cmd-v4l2.h   |    1 
>>>  pvrusb2-i2c-core.c       |   38 ++-
>>>  pvrusb2-sysfs.c          |   24 ++
>>>  pvrusb2-v4l2.c           |  100 +++++++++
>>>  12 files changed, 669 insertions(+), 100 deletions(-)
>>>
>>> Please note that I marked the first change (the USB ID fix) as high
>>> priority; it is trivial and an obvious right thing to do.
>>>       
>> OK.
>>
>> 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;
>> +	}
>>
>> Except for those tricks, the patches looks sane to my eyes. Please fix and ask me to pull again.
>>     
>
> Nope.  Sorry.  If I remove the "tricks", then the coding style 
> violations will come back.  You choose.  I have said this again and 
> again and again.  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 (or some other crazy bit 
> of debugging code).  I flat out refuse to use the first form.  I adopted 
> this habit over 20 years ago for a good reason, and BS like this isn't 
> going to change it.  I will put up with the space-after-comma silliness.  
> I will NOT put up with this.
>
> Those comments do NOT violate coding style.  You cannot have it both 
> ways.  Either I remove the comments and restore the "violations" or the 
> comments stand.  That is it.  No exceptions.  I have had enough with 
> this BS.  No more.

Mauro,

There is nothing wrong with adding a /* comment */ inside an if block,
even if that comment says, " /* this is the only way to pass
checkpatch.pl */ "

I fully agree with Mike, in that it is much safer to enclose all if
blocks within brackets.

I understand that kernel codingstyle forbids single line bracketing, but
codingstyle does not forbid adding comments anywhere in the c source.

Mike added a comment, to create a compromise between kernel codingstyle
and his own.  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.

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

Not only is this another example of checkpatch.pl thwarting development
-- this is sickness INSPIRED by checkpatch.pl that is not preventing a
merge -- what insanity.

Just merge his tree, please.

Regards,
Mike Krufky

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

[Stefan Richter]

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/

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

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

[Bill Davidsen]

Mike Isely wrote:

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

That's always a problem with humans in the loop. I very much agree that 
one line or three is far safer against a hasty line insertion than only two.

In my own code I write one if it fits, three if it doesn't. Being easy 
to read is good, being hard to misread is better.
> 
> 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.
> 

-- 
Bill Davidsen <davidsen@tmr.com>
   "We have more to fear from the bungling of the incompetent than from
the machinations of the wicked."  - from Slashdot