Google
  Web www.spinics.net

Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


On Tue, 2 Sep 2008, Hans Verkuil wrote:

> On Tuesday 02 September 2008 22:24:01 Mike Isely wrote:
> > On Tue, 2 Sep 2008, Hans Verkuil wrote:
> > 

   [...]

> > 
> > 
> > What?  Look at this:
> > 
> > > Don't put multiple statements on a single line unless you have
> > > something to hide:
> > > 
> > >         if (condition) do_this;
> > >           do_something_everytime;
> > > 
> > 
> > That's the case of:
> > 
> > 	if (a) b;
> > 
> > And yes, in that case you "do" have something to hide: "b", because it 
> > is conditionally executed based on "a".  That's the point of that rule.  
> > But run that by checkpatch.pl and it complains.
> 
> Hmm, I interpreted this as "don't do this, unless you want to pull some 
> dirty trick that you want to hide". This probably needs clarification.

But what could that possibly be?  Any "dirty trick" here is either going 
to be a gross bit of obfuscated code or at least another different kind 
of coding style violation.  I have always interpreted that to mean that 
it's "hiding" because it is technically not in the normal flow of 
execution.  That is why I see in the example "do_this;" in one place and 
"do_something_everytime;" in the other.  It's pointing out that the 
"do_this;" is different and doesn't run every time.  It's an exanple of 
something to hide.  What the above rule forbids would obviously be 
something like this

	do_something_everytime; do_something_everytime;

where the "hidden" statement is not any different and should stand on 
its own.

Really, the if-predicate plus the statement are logically their own 
statement in this case.  Makes perfect sense and I have seen plenty of 
very clear examples of code written this way.  Forcing that to become

	if (a)
		b;

to me just wastes space and opens one up for an error since at first 
glance those look like two statements when in fact it's really only one 
statement that's been broken into two lines.  Note that "if (a)" cannot 
stand by itself as a statement, it is tied to "b;".  But because the 
code still compiles even after accidentally inserting a printf() (or 
whatever you like) this because a subtle avenue for error.  This to me 
is why of the three possibilities

1.	if (a) b;

2.	if (a)
		b;


3.	if (a) {
		b;
	}

the second case is the worst of the lot.  Yet this is PRECISELY what 
checkpatch.pl forces people to do!  I see the CodingStyle phrasing as 
allowing the first case, and I think a strong argument can be made for 
also allowing the third case (if at least as a way of avoiding going 
past 80 columns without falling into the second case).


  [...]

> > the right things in the right places.  The checkpatch script has, for 
> me 
> > at least, two critical flaws. 
> 
> > (The other is space-after-comma.)  
> 
> Interesting, this one. I never understood why people would have problems 
> with that one. After all, when you write normal English you also put a 
> space after a comma. So why should code be any different?

This is one where either choice is not that "bad".  But for someone who 
has been doing it one way for 20 years, it is a big deal to try to force 
the other.  It's like telling a right-handed person to suddenly start 
eating with his left hand - you constantly find yourself slowed down and 
correcting yourself.  Code-wise the results are the same, and there's no 
correctness risk from one choice over the other (unlike the if-statement 
choices).

I have found one very mild reason to not have a space after a comma: 
When specifying arguments to function calls or preprocessor macros.  In 
those cases it saves a few character columns and makes it possible for 
something to fit in 80 columns that might not otherwise fit.  I find 
that readability-wise it simply makes no difference and is basically why 
a long long time ago I stopped using trailing spaces after commas.

The CodingStyle document provides a list of punctuation / formatting 
characters which require a trailing space.  The comma is not in that 
list.  I have to think that the omission of the comma in that case was a 
deliberate act.  However checkpatch.pl gets very noisy if you leave out 
the trailing space for any comma.

I find it painful and slow to keep correcting my code for this.  I find 
it even more annoying now that my driver has a mixture of this - even 
worse than just being uniformly "wrong".  I actually have a C program I 
wrote which can bulk-fix every C file.  But I don't want to use this 
because the resulting changeset will be, well, probably every single 
line of code in the driver.  So now I'm forced to live with this mixture 
and to keep hitting and correcting more cases of this as changes going 
forward touches more code.  What a stupid idiotic thing to do.

> 
> > I am  
> > finding myself worrying more about checkpatch than actually solving the 
> > 'real' problems at hand.  That is bad.  I am sick of it.  I do not know 
> > what I will do.  Probably I won't do anything pro-active, but if I 
> > continue to get flack about petty things like this then I will probably 
> > just mentally give up and go find less stressful things to do.
> 
> If you like then I can post to the lkml to ask for clarification and 
> rationale behind the
> 
> if (a) b;
> 
> vs.
> 
> if (a)
> 	b;
> 
> vs.
> 
> if (a) {
> 	b;
> }
> 
> coding style requirements.
> 
> The CodingStyle document is clearly not precise enough in the text.

Do whatever you like...

  -Mike


-- 

Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

_______________________________________________
v4l-dvb-maintainer mailing list
v4l-dvb-maintainer@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/v4l-dvb-maintainer

[Linux Media]     [Older V4L]     [Linux DVB]     [Video Disk Recorder]     [Asterisk]     [Photo]     [DCCP]     [Netdev]     [Xorg]     [Util Linux NG]     [Xfree86]     [Free Photo Albums]     [Fedora Users]     [Fedora Women]     [ALSA Users]     [ALSA Devel]     [SSH]     [Linux USB]

-->
Add to Google Powered by Linux

Google PageRank Checking tool