Hey hackers. ;)
I created this little patch while reading through drivers/block/ub.c. It fixes
some indentation/whitespace as well as some empty commenting, an hardcoded
module name and an unneeded return.
The patch is against the latest -git.
Marc
On Wed, 8 Feb 2006 10:29:15 +0100, Marc Koschewski <[email protected]> wrote:
> I created this little patch while reading through drivers/block/ub.c. It fixes
> some indentation/whitespace as well as some empty commenting, an hardcoded
> module name and an unneeded return.
I strongly disagree.
The only segment which has some merit is the one which replaces the .name
with DRV_NAME. It could have been "usb-block" or something, but we probably
won't be using that, so it's all right.
But the rest is quite bad, the worst being indenting the switch statement.
Is there nothing else you can do in the whole kernel?
-- Pete
* Pete Zaitcev <[email protected]> [2006-02-08 19:40:57 -0800]:
> On Wed, 8 Feb 2006 10:29:15 +0100, Marc Koschewski <[email protected]> wrote:
>
> > I created this little patch while reading through drivers/block/ub.c. It fixes
> > some indentation/whitespace as well as some empty commenting, an hardcoded
> > module name and an unneeded return.
>
> I strongly disagree.
OK.
>
> The only segment which has some merit is the one which replaces the .name
> with DRV_NAME. It could have been "usb-block" or something, but we probably
> won't be using that, so it's all right.
I just dislike using the same values twice and not using constants. More of a
personal thing I guess.
>
> But the rest is quite bad, the worst being indenting the switch statement.
I see. Why do you use if-statement-indenting then?
What is the sense of the empty comments? What sense does the 'immediate
return' make when the value is returned 2 lines below (where one line is just
a closing brace)?
>
> Is there nothing else you can do in the whole kernel?
Sure, but I guess you don't have to tell me what files I put my nose in, do
you? I didn't mean to personally piss you off by sending this in. Tzzz ...
Unfortunately my understanding of how ie. the Linux VM works is not very good.
In fact it's poor. But I will dig into this and try to make even you happy with
a patch that makes sense _in your eyes_.
>
> -- Pete
>
Marc
On Thu, 9 Feb 2006 10:21:43 +0100, Marc Koschewski <[email protected]> wrote:
> > But the rest is quite bad, the worst being indenting the switch statement.
>
> I see. Why do you use if-statement-indenting then?
Insides of case blocks are indented, didn't you notice? Why waste a tab?
> What is the sense of the empty comments? What sense does the 'immediate
> return' make when the value is returned 2 lines below (where one line is just
> a closing brace)?
Empty comments segregate related groups of functions.
The return is there in case something else is needed between the if and
the function's end. It's there to underscore the regular structure of the
code. Though the big block comment obscures the intent there. Perhaps
I should relocate it.
You didn't mention empty lines on top of functions. They appear where
variables are likely to be, or if the function body has a break, to make
it fair to all little segments :-)
> > Is there nothing else you can do in the whole kernel?
>
> Sure, but I guess you don't have to tell me what files I put my nose in, do
> you? I didn't mean to personally piss you off by sending this in. Tzzz ...
>
> Unfortunately my understanding of how ie. the Linux VM works is not very good.
> In fact it's poor. But I will dig into this and try to make even you happy with
> a patch that makes sense _in your eyes_.
I don't know anything about VM either and I do not see how this is
relevant.
The bigger point is, if you strive to make a meaningful contribution,
meaningless code rearrangements is not the way to go about it. And
we have plenty of work under the subsystem level, though usually it
has something to do with specific hardware. For example, Firewire
appears to be in dire need of fixing right now.
I remember a period of time when the janitoring project produced
meaningful cleanups, such as verification of failure paths. This work
is still relevant. Just the other day I sent a patch to Dmitry for
a leak in mousedev. And remember the debacle of memset(p, size, 0)!
I'm not going to tell you where to put your nose in, but I would like
to hint that the janitoring project might use some help with something
going beyond the uglification of my pretty code :-)
-- Pete
>> I created this little patch while reading through drivers/block/ub.c. It fixes
>> some indentation/whitespace as well as some empty commenting, an hardcoded
>> module name and an unneeded return.
>
>I strongly disagree.
>
>But the rest is quite bad, the worst being indenting the switch statement.
>
switch(a) {
case A:
dosomething;
break;
}
Now what if
switch(a) {
case A: {
int tmp;
do_something;
break;
}
}
Jan Engelhardt
--
* Pete Zaitcev <[email protected]> [2006-02-09 08:55:01 -0800]:
> On Thu, 9 Feb 2006 10:21:43 +0100, Marc Koschewski <[email protected]> wrote:
>
> > > But the rest is quite bad, the worst being indenting the switch statement.
> >
> > I see. Why do you use if-statement-indenting then?
>
> Insides of case blocks are indented, didn't you notice? Why waste a tab?
The 'case' block are, however, _within_ the switch, thus they need to be
indented - just my 2 cents. But OK ...
>
> > What is the sense of the empty comments? What sense does the 'immediate
> > return' make when the value is returned 2 lines below (where one line is just
> > a closing brace)?
>
> Empty comments segregate related groups of functions.
>
Didn't know. Is there any documentation on this _really_ helpful mechanism?
>
> The return is there in case something else is needed between the if and
OK, so why is _any_
if (1)
get_lost;
stuff left there without the braces? I guess one day there _could_ another
line to lead to
if (1) {
beat_it;
get_lost;
}
where the braces are definitely missing. You could also have pinned the return
there when it was needed. And for now it's definitely not. Maybe for readability.
But that's another thing...
> the function's end. It's there to underscore the regular structure of the
> code. Though the big block comment obscures the intent there. Perhaps
> I should relocate it.
.. to be discussed right here. You're snippet is not _more readable_ due to the
return. Any when it comes to such things as 'return the rc right now as we're
setup and ready' I could point out some other locations where the rc is set, the
code did its setup for ie. a device and many if-statements follow but no change
to rc is made. Why not use 'immediate return' there?
Maybe on could clarify for me.
>
> You didn't mention empty lines on top of functions. They appear where
> variables are likely to be, or if the function body has a break, to make
> it fair to all little segments :-)
I did understand the fact that one line-breaks the function declaration. But I
don't understand the need to put a blank line where someday a var could
defined.
I could put another 60 blank lines somewhere just in case some vendor brings any
device that needs some special treatment on init or something.
>
> > > Is there nothing else you can do in the whole kernel?
> >
> > Sure, but I guess you don't have to tell me what files I put my nose in, do
> > you? I didn't mean to personally piss you off by sending this in. Tzzz ...
> >
> > Unfortunately my understanding of how ie. the Linux VM works is not very good.
> > In fact it's poor. But I will dig into this and try to make even you happy with
> > a patch that makes sense _in your eyes_.
>
> I don't know anything about VM either and I do not see how this is
> relevant.
It was just an example. To me it looks like some sorta 'this is my code and you
please let your finger off it'-thing. I do understand that whitespace/indenting-only
patches are just about to get us in trouble when it comes to other patches
applied to the tree as ie. Andi Kleen stated in 'Re: [PATCH] early_printk:
cleanup trailiing whitespace' today.
The remove-return-thing, however, was not a whitespace thing. I still disagree
it makes the code more readable or whatever. It's just redundant as this time.
When there's a need, we could add it.
>
> The bigger point is, if you strive to make a meaningful contribution,
> meaningless code rearrangements is not the way to go about it. And
> we have plenty of work under the subsystem level, though usually it
> has something to do with specific hardware. For example, Firewire
> appears to be in dire need of fixing right now.
OK, let's face it: I don't have any FireWire hardware around. Not even a cable.
So I'm not gonna do anyting there.
I just stumbled upon the ub.c code on my way to get my USB chipcard reader module
from crashing when it's unloaded. The patch just exists because I read the file
and wondered what all these comments should 'comment' if now comment was
actually in there.
>
> I remember a period of time when the janitoring project produced
> meaningful cleanups, such as verification of failure paths. This work
> is still relevant. Just the other day I sent a patch to Dmitry for
> a leak in mousedev. And remember the debacle of memset(p, size, 0)!
> I'm not going to tell you where to put your nose in, but I would like
> to hint that the janitoring project might use some help with something
> going beyond the uglification of my pretty code :-)
Could you point me to some location?
>
> -- Pete
>
Marc
On Thu, 9 Feb 2006 18:24:20 +0100 (MET), Jan Engelhardt <[email protected]> wrote:
> Now what if
>
> switch(a) {
> case A: {
> int tmp;
> do_something;
> break;
> }
> }
Not in _my_ code.
-- Pete