Why keeping lines commented out via #if 0?
Hi!
I recently grep'd through the kernel sources and found *a lot* of lines that are commented out using #if 0. Is there any reason to keep them? I think that lines that are commented out should be removed from the mainline kernel. If developers need them in future, they can create a local copy or branch on their machines. Or am I missing the point about the whole #if 0 thing?
Andi
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
Quoted 9 lines · Andi Drebes
Hi!
I recently grep'd through the kernel sources and found *a lot* of lines that are commented out using #if 0. Is there any reason to keep them? I think that lines that are commented out should be removed from the mainline kernel. If developers need them in future, they can create a local copy or branch on their machines. Or am I missing the point about the whole #if 0 thing?Andi
been there, done that:
http://fsdev.net/wiki/index.php?title=If_0_or_if_1_directives
rday
Signature · 11 lines
==================================== Robert P. J. Day Linux Consulting, Training and Annoying Kernel Pedantry Waterloo, Ontario, CANADA http://fsdev.net/wiki/index.php?title=Main_Page ==================================== _______________________________________________ Kernel-janitors mailing list [email protected] https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
Quoted 2 lines · Andi Drebes
I recently grep'd through the kernel sources and found *a lot* of lines that are commented out using #if 0. Is there any reason to keep them?
Yes -- the people who wrote the code originally put them there. Unless you're willing to go through each one, and figure out whether it's still useful or not for anyone reading this section of code later, it's better to leave it alone.
Why are comments considered useful, yet commented-out sections of code considered worthy of deletion? The latter are potentially more useful. Of course things can be taken too far and #if 0 / #if 1 sections can clutter up the function and make it harder to understand. But each one needs to be taken in context and understood, not mindlessly excised.
I think the kernel-janitors need a Hypocritic Oath. First, do no harm. I'm a little sensitive to so-called 'trivial' changes right now, given that Kyle just spent two days tracking down breakages on parisc from these kinds of patches.
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
Quoted 7 lines · Matthew Wilcox
Quoted 3 lines · Andi Drebes
I recently grep'd through the kernel sources and found *a lot* of lines that are commented out using #if 0. Is there any reason to keep them?
Yes -- the people who wrote the code originally put them there.
with all due respect, matthew, that's an idiotic answer.
Quoted 3 lines
Unless you're willing to go through each one, and figure out whether it's still useful or not for anyone reading this section of code later, it's better to leave it alone.
it's not clear that andi was suggesting a single, sweeping deletion operation (which *would* be a bad idea). when i read his post, *i* interpreted it as suggesting that a gradual examination might be in order, and stuff that was clearly worthless could be removed bit by bit.
Quoted 2 lines
Why are comments considered useful, yet commented-out sections of code considered worthy of deletion?
gee, matt, i dunno ... could it be that, if some code has been deleted via "#if 0", it might actually no longer have any value? could *that* be it?
Quoted 2 lines
Of course things can be taken too far and #if 0 / #if 1 sections can clutter up the function and make it harder to understand.
good boy, matt ... thanks for clueing in eventually.
Quoted 2 lines
But each one needs to be taken in context and understood, not mindlessly excised.
which is how i read andi's post.
Quoted 4 lines
I think the kernel-janitors need a Hypocritic Oath. First, do no harm. I'm a little sensitive to so-called 'trivial' changes right now, given that Kyle just spent two days tracking down breakages on parisc from these kinds of patches.
first, it's "hippocratic". second, if patches are submitted by janitors that cause actual breakage, then i suggest it's the fault of the official maintainers who eventually *applied* those patches. few janitors have the authority to *force* a patch on anyone's tree, so if a KJ patch f**ks something up, i suggest the blame lies elsewhere.
and, finally, a lot of the work being done by janitors wouldn't be necessary if the actual developers invested just a bit of time in keeping their code relatively clean. instead, crappy and slowly-rotting code is allowed to accumulate in the tree to the point where you have (and i speak from personal experience):
- misspelled and/or long-dead CONFIG variables
- long-deceased header files
- source files that are never used as part of the build under *any* circumstances
- and, as above, hundreds upon hundreds of "#if 0" constructs, many of which i'm sure have been there for years and have absolutely no value
i submit, matthew, that there wouldn't even be a *need* for a kernel janitors project if so many developers weren't so lazy and/or sloppy. but, sadly, they are, and the annoying part of all of this is that a lot of those developers are quite happy to leave the scut work to the janitors, but get all excited and put out when the occasional patches are submitted that cause breakage.
if folks like you are so irked by broken patches, then the solution is simple -- fix all your own code and don't accept anything from janitors. but as long as you feel like leaving that sort of maintenance to people who offer to be janitors, then you should be prepared to deal with the occasional bad patch.
and, you know, saying "thank you" for all that janitorial effort every so often probably wouldn't be out of line, either.
rday
Signature · 11 lines
==================================== Robert P. J. Day Linux Consulting, Training and Annoying Kernel Pedantry Waterloo, Ontario, CANADA http://fsdev.net/wiki/index.php?title=Main_Page ==================================== _______________________________________________ Kernel-janitors mailing list [email protected] https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
Quoted 1 line · Robert P. J. Day
first, it's "hippocratic".
Thanks for failing to get the joke.
Quoted 5 lines
second, if patches are submitted by
janitors that cause actual breakage, then i suggest it's the fault of the official maintainers who eventually *applied* those patches. few janitors have the authority to *force* a patch on anyone's tree, so if a KJ patch f**ks something up, i suggest the blame lies elsewhere.
Gotcha. Janitors are perfect, people who actually implement features and fix bugs are sloppy and idiots.
Quoted 7 lines
i submit, matthew, that there wouldn't even be a *need* for a kernel janitors project if so many developers weren't so lazy and/or sloppy. but, sadly, they are, and the annoying part of all of this is that a lot of those developers are quite happy to leave the scut work to the janitors, but get all excited and put out when the occasional patches are submitted that cause breakage.
Quoted 8 lines
if folks like you are so irked by broken patches, then the solution is simple -- fix all your own code and don't accept anything from janitors. but as long as you feel like leaving that sort of maintenance to people who offer to be janitors, then you should be prepared to deal with the occasional bad patch.
and, you know, saying "thank you" for all that janitorial effort every so often probably wouldn't be out of line, either.
I appreciate you're just starting out, and doing janitorial patches are a great way to do that, but you really need to either lose the attitude, or do something to earn it.
I'll even make you an offer -- want to become the ncr53c8xx maintainer? It's in use on a whole lot of older parisc machines, and needs a fairly significant amount of love. There's lots of commonality with the sym2 driver, so we can cooperate and share ideas. You probably don't have hardware, so I'll also commit to getting some hardware to you and/or access to machines with remote console and power management.
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
andi:
some thoughts on how much of the "#if 0" stuff can be removed.
first, in a lot of cases, there's little you can do. there will be some code that has been commented out, with no indication as to why that was done. if you're lucky, you might be able to recognize that the "removed" code represents a clearly deprecated or obsolete kernel construct, in which case you can certainly take a shot at putting together a patch and just emailing it offline to the respective subsystem maintainer.
i've had good luck with that -- if you're not sure about the patch,
there's no point posting it to the whole list, just send it to the maintainer and ask for some feedback. in some cases, they'll just add it to their local tree and you're done.
in other cases, there's obviously something weird going on and, if
you want to follow up, you might need to do some detective work. as an example, consider this snippet from arch/blackfin/oprofile/common.c:
...
static void op_bfin_shutdown(void)
{
#if 0
/* what is the difference between shutdown and stop? */#endif
}
...
static void op_bfin_stop(void)
{
mutex_lock(&pfmon_lock);
if (pfmon_enabled) {
model->stop();
pfmon_enabled = 0;
}
mutex_unlock(&pfmon_lock);
}...
i don't know -- what *is* the difference? you'd think the official
blackfin maintainer should know something like that. and if you grep under arch/blackfin, it certainly seems like no one is terribly interested in that routine (at least under that directory):
$ grep -rw shutdown * mach-common/ints-priority-dc.c: .shutdown = bf561_gpio_irq_shutdown mach-common/ints-priority-sc.c: .shutdown = bfin_gpio_irq_shutdown oprofile/common.c: /* what is the difference between shutdown and stop? */ oprofile/common.c: ops->shutdown = op_bfin_shutdown;
however, without that particular system, it's tough to compile test
a removal. in cases like that, a short email to one of the maintainers might clear things up.
there's no one formula for this sort of cleanup, and it's typically
compounded by the fact that developers are outrageously possessive of their code, even if it's years old and rotting. best advice: just start perusing and pick something that looks obvious.
rday
Signature · 11 lines
==================================== Robert P. J. Day Linux Consulting, Training and Annoying Kernel Pedantry Waterloo, Ontario, CANADA http://fsdev.net/wiki/index.php?title=Main_Page ==================================== _______________________________________________ Kernel-janitors mailing list [email protected] https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
Quoted 4 lines
some thoughts on how much of the "#if 0" stuff can be removed.
first, in a lot of cases, there's little you can do. there will be some code that has been commented out, with no indication as to why that was done.
ok, now I'm cautious with my comments because the discussion became quite emotional (the change of views between Matthew and you). However, I think that if there's no indication why the code was commented out and if even the maintainer doesn't know why, I think it is quite clear that the affected lines should be removed, because the collective memory of the community has forgotten why it was done. If nobody knows it, I think that nobody will ever use this code again.
Quoted 9 lines
if you're lucky, you might be able to recognize that
the "removed" code represents a clearly deprecated or obsolete kernel construct, in which case you can certainly take a shot at putting together a patch and just emailing it offline to the respective subsystem maintainer.
i've had good luck with that -- if you're not sure about the patch,
there's no point posting it to the whole list, just send it to the maintainer and ask for some feedback. in some cases, they'll just add it to their local tree and you're done.
The next time I encounter lines commented out with #if 0 and I detect something like that, I'll follow your advice.
Quoted 2 lines
in other cases, there's obviously something weird going on and, if
you want to follow up, you might need to do some detective work.
<snip>
Quoted 2 lines
in cases like that, a short email to one of the
maintainers might clear things up.
ok.
Quoted 4 lines
there's no one formula for this sort of cleanup, and it's typically
compounded by the fact that developers are outrageously possessive of their code, even if it's years old and rotting. best advice: just start perusing and pick something that looks obvious.
ok.
Andi
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
Quoted 11 lines · Andi Drebes
Quoted 4 lines
some thoughts on how much of the "#if 0" stuff can be removed.
first, in a lot of cases, there's little you can do. there will be some code that has been commented out, with no indication as to why that was done.ok, now I'm cautious with my comments because the discussion became quite emotional (the change of views between Matthew and you). However, I think that if there's no indication why the code was commented out and if even the maintainer doesn't know why, I think it is quite clear that the affected lines should be removed, because the collective memory of the community has forgotten why it was done. If nobody knows it, I think that nobody will ever use this code again.
And we have source control management these days, so I think this specific cleanup case is not that contentious.
But please add a comment when somebody remembers why the #if 0 was put there :-)
- Arnaldo
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
<snip>
Quoted 9 lines
Quoted 3 lines
Unless you're willing to go through each one, and figure out whether it's still useful or not for anyone reading this section of code later, it's better to leave it alone.
it's not clear that andi was suggesting a single, sweeping deletion operation (which *would* be a bad idea). when i read his post, *i* interpreted it as suggesting that a gradual examination might be in order, and stuff that was clearly worthless could be removed bit by bit.
<snip>
To clarify that affair: No, I didn't want to write a big patch that removes those lines. I just aked for a reason why those lines should be kept in the tree. Even if I had not asked why, I wouldn't have written such a patch. Instead I would have inspected these lines in detail (which is btw what I'm going to do whenever I encounter such lines in the future). Perhaps I picked up the wrong list. Maybe I should have asked that on the kernel-newbie mailing list.
I'm sorry this conversation got that emotional. I didn't want to bring you to that point (please remember: it was just a *question*).
Cheers,
Andi
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
2007/6/2, Andi Drebes <[email protected]>:
Quoted 19 lines
<snip>
Quoted 9 lines
Quoted 3 lines
Unless you're willing to go through each one, and figure out whether it's still useful or not for anyone reading this section of code later, it's better to leave it alone.
it's not clear that andi was suggesting a single, sweeping deletion operation (which *would* be a bad idea). when i read his post, *i* interpreted it as suggesting that a gradual examination might be in order, and stuff that was clearly worthless could be removed bit by bit.
<snip>
To clarify that affair: No, I didn't want to write a big patch that removes those lines. I just aked for a reason why those lines should be kept in the tree. Even if I had not asked why, I wouldn't have written such a patch. Instead I would have inspected these lines in detail (which is btw what I'm going to do whenever I encounter such lines in the future). Perhaps I picked up the wrong list. Maybe I should have asked that on the kernel-newbie mailing list.
Quoted 2 lines
I'm sorry this conversation got that emotional. I didn't want to bring you to that point (please remember: it was just a *question*).
Hi,
I don't think you should feel guilty about that, that was something between Matthew and Robert.
I personally think the tone of Robert's mail was somehow unnecessarily aggressive or attacking against Matthew, even though I tend to agree with some of the points mentioned.
Thomas
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
Quoted 6 lines · Andi Drebes
To clarify that affair: No, I didn't want to write a big patch that removes those lines. I just aked for a reason why those lines should be kept in the tree. Even if I had not asked why, I wouldn't have written such a patch. Instead I would have inspected these lines in detail (which is btw what I'm going to do whenever I encounter such lines in the future).
i doubt it's a good investment of time to *explicitly* try to remove stuff enclosed in "#if 0" -- you're probably better off picking a specific item or topic from the list of projects and working on that, unless you just happen to stumble across commented out stuff that looks like a good candidate for removal.
in addition, make sure you have a good idea of what portions of the source tree are already slated for removal, and don't waste any time there. for example, there's no point in doing *any* cleanup of the entire OSS sound system, since that's going to be tossed *entirely* someday. (but don't base that kind of decision on the feature removal file Documentation/feature-removal-schedule.txt -- that file is virtually worthless in terms of accurate information content.)
one strategy i've used on occasion when i'm bored is to just search the entire tree looking for the words "deprecated" or "obsolete" and seeing what turns up.
in any event, i should probably spend a few minutes and clean up the project list here:
http://fsdev.net/wiki/index.php?title=Kernel_Janitor%27s_Todo_List
to remove items that look like they're basically done.
rday
Signature · 11 lines
==================================== Robert P. J. Day Linux Consulting, Training and Annoying Kernel Pedantry Waterloo, Ontario, CANADA http://fsdev.net/wiki/index.php?title=Main_Page ==================================== _______________________________________________ Kernel-janitors mailing list [email protected] https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
Quoted 8 lines · Robert P. J. Day
Quoted 2 lines · Matthew Wilcox
Why are comments considered useful, yet commented-out sections of code considered worthy of deletion?
gee, matt, i dunno ... could it be that, if some code has been deleted via "#if 0", it might actually no longer have any value? could *that* be it?
No ofcourse it could not. If it didn't have any value any longer it would have been deleted, not turned into a comment.
The value #if 0 code generally retains is documentation. Documentation on how things were done before and/or could be done in the future, but are not done now. It's documentation for the current developer but even more importantly, a future developer. Any "I tried this, but this didn't work quite right yet" or "I'd like to do it this way, but this needs something else first" or similar information this conveys is very useful, especially over maintainer changes.
Ofcourse, noone should go overboard with it but people don't. If something has no value anymore, it's deleted. It's only kept as a comment when it _is_ seen to have value in some or other form and as such deleting it is not a janitorial task but something that needs a maintainer. If the code sees active maintainance it still being there already means it's seen to still have value and if the code does not, it should most certainly be left alone as documentation for a possible next person stepping in.
A suggestion that any #if 0 code should preferably have a comment describing why it's there would ofcourse not be bad.
Rene.
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
<snip>
Quoted 10 lines
Ofcourse, noone should go overboard with it but people don't. If something has no value anymore, it's deleted. It's only kept as a comment when it _is_ seen to have value in some or other form and as such deleting it is not a janitorial task but something that needs a maintainer. If the code sees active maintainance it still being there already means it's seen to still have value and if the code does not, it should most certainly be left alone as documentation for a possible next person stepping in.
A suggestion that any #if 0 code should preferably have a comment describing why it's there would ofcourse not be bad.
Yes, I totally agree with that. Especially for people that are new to kernel stuff (me, for example) are quite confused about the fact that there's so much code commented out. Just a short and descriptive line that tells people why this was commented out would be very nice and helpful.
Andi
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
Quoted 21 lines · Rene Herman
Quoted 8 lines · Robert P. J. Day
Quoted 2 lines · Matthew Wilcox
Why are comments considered useful, yet commented-out sections of code considered worthy of deletion?
gee, matt, i dunno ... could it be that, if some code has been deleted via "#if 0", it might actually no longer have any value? could *that* be it?
No ofcourse it could not. If it didn't have any value any longer it would have been deleted, not turned into a comment.
The value #if 0 code generally retains is documentation. Documentation on how things were done before and/or could be done in the future, but are not done now. It's documentation for the current developer but even more importantly, a future developer. Any "I tried this, but this didn't work quite right yet" or "I'd like to do it this way, but this needs something else first" or similar information this conveys is very useful, especially over maintainer changes.
Quoted 2 lines
A suggestion that any #if 0 code should preferably have a comment describing why it's there would ofcourse not be bad.
In addition, I would like to mention that documentation of these #if 0s can sometimes be found in the changelogs. For a totally random example:
$ grep -rn "^#if 0$" kernel/ kernel/params.c:27:#if 0 kernel/params.c:722:#if 0 kernel/module.c:51:#if 0 kernel/lockdep.c:204:#if 0
Now, inspecting this can be as easy as running git annotate on the file/lineno pair. Try for instance:
$ git annotate -l -L 722 kernel/params.c
And it shows who made the change and which commit it was. In this case, git says ef665c1a06be719ed9a6b0ad7967137258d9457a, which can be further inspected using git log:
$ git log -p -n 1 ef665c1a06be719ed9a6b0ad7967137258d9457a commit ef665c1a06be719ed9a6b0ad7967137258d9457a Author: Randy Dunlap <[email protected]> Date: Tue Feb 13 15:19:06 2007 -0800
sysfs: fix build errors: uevent with CONFIG_SYSFS=n
Fix source files to build with CONFIG_SYSFS=n. module_subsys is not available.
SYSFS=n, MODULES=y: T:y
SYSFS=n, MODULES=n: T:y
SYSFS=y, MODULES=y: T:y
SYSFS=y, MODULES=n: T:y
So the #if 0 is simply a build fix where the old code is kept for reference. I guess the sysfs_ops was at one point defined even though CONFIG_SYSFS was unset, but I don't know. Personally, I don't see any point in keeping the #if 0ed code. Changing it to #if 1 wouldn't even make a difference at this point, since the variable is unused. But it can at least be useful to have the commit number for reference and CC the authors of that particular commit when submitting a patch for it.
Kind regards,
Vegard
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
it's not the "#if 0"s that amuse me ... it's the "#if 1"s:
$ grep -r "^#if 1$" * | wc -l 226 $
some of them have some real entertainment value:
...
#if 1 /* XXX screw this, I can't get the VAC flushes working
* XXX reliably... -DaveM
*/
.globl swift_flush_cache_all, swift_flush_cache_mm .globl swift_flush_cache_range, swift_flush_cache_page .globl swift_flush_page_for_dma
.globl swift_flush_page_to_ram
swift_flush_cache_all:swift_flush_cache_mm:swift_flush_cache_range:swift_flush_cache_page:swift_flush_page_for_dma:swift_flush_page_to_ram:
sethi %hi(0x2000), %o0
1: subcc %o0, 0x10, %o0
add %o0, %o0, %o1
sta %g0, [%o0] ASI_M_DATAC_TAG
bne 1b
sta %g0, [%o1] ASI_M_TXTC_TAG retl
nop
#else
...
rday
Signature · 11 lines
==================================== Robert P. J. Day Linux Consulting, Training and Annoying Kernel Pedantry Waterloo, Ontario, CANADA http://fsdev.net/wiki/index.php?title=Main_Page ==================================== _______________________________________________ Kernel-janitors mailing list [email protected] https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors