CONFIG_* In Comments Considered Harmful
PATCH 1/1 CONFIG_* In Comments Considered Harmful
I reviewed the dependency list for a file this morning to see why it was being unnecessarily recompiled (a little fetish of mine, mostly harmless). I was a little discombobulated to find this line:
$(wildcard include/config/higmem.h) \
Naturally, I assumed a typo somewhere. It turns out there is indeed a CONFIG_HIGMEM in include/linux/mm.h, but it's in a comment. The fixdep script doesn't parse C itself, so it doesn't know that this should be ignored. Rather than fix the typo, I deleted the comment; the ifdef'ed code is a mere two lines so the comment seems unnecessary.
This serves as a useful warning to people -- don't put CONFIG_FOO in a comment unnecessarily. Because even when it's true now, maybe the #if gets changed and the comment doesn't.
Index: include/linux/mm.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/linux/mm.h,v
retrieving revision 1.5
include/linux/mm.h +1 -1
@@ -196,7 +196,7 @@ struct page {#if defined(WANT_PAGE_VIRTUAL )void *virtual;/* Kernel virtual address (NULL if not kmapped, ie. highmem) */ -#endif /* CONFIG_HIGMEM || WANT_PAGE_VIRTUAL */ +#endif };/*
Signature · 3 lines
"It's not Hollywood. War is real, war is primarily not about defeat or victory, it is about death. I've seen thousands and thousands of dead bodies. Do you think I want to have an academic debate on this subject?" -- Robert Fisk
Quoted 4 lines · Matthew Wilcox
I reviewed the dependency list for a file this morning to see why it was being unnecessarily recompiled (a little fetish of mine, mostly harmless). I was a little discombobulated to find this line:
Mmm discombobulation.
Quoted 6 lines
$(wildcard include/config/higmem.h) \Naturally, I assumed a typo somewhere. It turns out there is indeed a
CONFIG_HIGMEMininclude/linux/mm.h, but it's in a comment. The fixdep script doesn't parse C itself, so it doesn't know that this should be ignored.
Maybe it should be taught to parse comments? There are zillions of #endif /* CONFIG_FOO */
braces in the tree. Why is this one special ?
Dave
Signature · 1 line
Dave Jones http://www.codemonkey.org.uk
Quoted 3 lines · Dave Jones
Maybe it should be taught to parse comments? There are zillions of #endif /*
CONFIG_FOO*/
braces in the tree. Why is this one special ?
I think it's because it looked like:
#ifdef CONFIG_FOO
....
#endif /* CONFIG_FOO or CONFIG_BAR */
and it concluded there was a dependency on BAR.
Quoted 6 lines · On Wed, 1 October 2003 10:29:55 -0400, [email protected] wrote:
Quoted 3 lines · Dave Jones
Maybe it should be taught to parse comments? There are zillions of #endif /*
CONFIG_FOO*/
braces in the tree. Why is this one special ?
Quoted 7 lines
I think it's because it looked like:
#ifdef
CONFIG_FOO
....
#endif /*CONFIG_FOOorCONFIG_BAR*/and it concluded there was a dependency on BAR.
Or rather like this:
#ifdef CONFIG_FOO
...
#endif /* CONFIG_FO */
Problem is that we humans correct the type before it even reaches our conciousness. Machines don't do that yet.
Jörn
Signature · 2 lines
The strong give up and move away, while the weak give up and stay. -- unknown
Quoted 15 lines · Jörn Engel
Quoted 6 lines · On Wed, 1 October 2003 10:29:55 -0400, [email protected] wrote:
Quoted 3 lines · Dave Jones
Maybe it should be taught to parse comments? There are zillions of #endif /*
CONFIG_FOO*/
braces in the tree. Why is this one special ?Quoted 7 lines
I think it's because it looked like:
#ifdef
CONFIG_FOO
....
#endif /*CONFIG_FOOorCONFIG_BAR*/and it concluded there was a dependency on BAR.
Quoted 8 lines
Or rather like this:
#ifdef
CONFIG_FOO
...
#endif /*CONFIG_FO*/Problem is that we humans correct the type before it even reaches our conciousness. Machines don't do that yet.
But it used to look like
#ifdef BAR || BAZ
...
#endif /* BAR || BAZ */
now it looks like
#ifdef BAZ
...
#endif /* BAR || BAZ */
if you can't trust people to keep comments up to date, delete the comments. No comments are better than wrong comments.
Remember the classic?
/* Keep these two variables together */
int bar;
Signature · 3 lines
"It's not Hollywood. War is real, war is primarily not about defeat or victory, it is about death. I've seen thousands and thousands of dead bodies. Do you think I want to have an academic debate on this subject?" -- Robert Fisk
Quoted 3 lines · Dave Jones
Maybe it should be taught to parse comments? There are zillions of #endif /*
CONFIG_FOO*/
braces in the tree. Why is this one special ?
I wondered the same, until I noticed:
#if defined(WANT_PAGE_VIRTUAL)
^^^^^^^^^^^^^^^^^
void *virtual; /* Kernel virtual address (NULL if
not kmapped, ie. highmem) */
#endif /* CONFIG_HIGMEM || WANT_PAGE_VIRTUAL */
^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^the #defined check is on WANT_PAGE_VIRTUAL only - the script tripped over CONFIG_HIGMEM, which is in the #endif only.
Signature · 2 lines
Muli Ben-Yehuda http://www.mulix.org
Quoted 25 lines · Matthew Wilcox
I reviewed the dependency list for a file this morning to see why it was being unnecessarily recompiled (a little fetish of mine, mostly harmless). I was a little discombobulated to find this line:
$(wildcard include/config/higmem.h) \Naturally, I assumed a typo somewhere. It turns out there is indeed a
CONFIG_HIGMEMininclude/linux/mm.h, but it's in a comment. The fixdep script doesn't parse C itself, so it doesn't know that this should be ignored. Rather than fix the typo, I deleted the comment; the ifdef'ed code is a mere two lines so the comment seems unnecessary.This serves as a useful warning to people -- don't put
CONFIG_FOOin a comment unnecessarily. Because even when it's true now, maybe the #if gets changed and the comment doesn't.Index:
include/linux/mm.h
===================================================================
RCS file:/var/cvs/linux-2.6/include/linux/mm.h,v
retrieving revision 1.5
diff -u -p -r1.5mm.h
include/linux/mm.h struct page { · line 200
Quoted 6 lines
include/linux/mm.h +1
-1@@ -196,7 +196,7 @@ struct page {#if defined(WANT_PAGE_VIRTUAL )void *virtual;/* Kernel virtual address (NULL if not kmapped, ie. highmem) */ -#endif /* CONFIG_HIGMEM || WANT_PAGE_VIRTUAL */ +#endif };
To me, this sounds more like our tools are too stupid. Ok, I don't like to copy&paste conditions either, but in many header files, those comments are the lesser evil. If we cannot put every #endif on the same screen (24 lines) as the corresponding #if, they do help.
What we do need is a version of cpp, that only removes comments. Then we can pipe all input for our tools through that cppp or whatever it would be called, and noone would worry about comments confusing the tools anymore.
All right, here is a patch that simply corrects the typo. Linus, you decide which one is better.
Index: include/linux/mm.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/linux/mm.h,v
retrieving revision 1.5
include/linux/mm.h +1 -1
@@ -196,7 +196,7 @@ struct page {#if defined(WANT_PAGE_VIRTUAL )void *virtual;/* Kernel virtual address (NULL if not kmapped, ie. highmem) */ -#endif /* CONFIG_HIGMEM || WANT_PAGE_VIRTUAL */ +#endif /* CONFIG_HIGHMEM || WANT_PAGE_VIRTUAL */ };
Jörn
Signature · 2 lines
The only real mistake is the one from which we learn nothing. -- John Powell
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 10 lines · Muli Ben-Yehuda
Quoted 10 lines
Index:
include/linux/mm.h
===================================================================
RCS file:/var/cvs/linux-2.6/include/linux/mm.h,v
retrieving revision 1.5
diff -u -p -r1.5mm.hinclude/linux/mm.h +0
-0@@ -196,7 +196,7 @@ struct page {#if defined(WANT_PAGE_VIRTUAL )
Quoted 8 lines
include/linux/mm.h struct page { · line 200
Quoted 5 lines
include/linux/mm.h +1
-1void *virtual;/* Kernel virtual address (NULL if not kmapped, ie. highmem) */ -#endif /* CONFIG_HIGMEM || WANT_PAGE_VIRTUAL */ +#endif /* CONFIG_HIGHMEM || WANT_PAGE_VIRTUAL */ };This is broken, the CONFIG_HIG(H)MEM shouldn't be there at all. Look at the #if defined(...) line.
include/linux/mm.h:353:
#if defined(CONFIG_HIGHMEM) && !defined(WANT_PAGE_VIRTUAL) include/linux/mm.h:199:
#endif /* CONFIG_HIGMEM || WANT_PAGE_VIRTUAL */
Ah, crud! The comment fits perfectly, it just sits at the wrong position. Agreed, no comments are better than wrong comments.
Scrap my patch.
Jörn
Signature · 2 lines
A defeated army first battles and then seeks victory. -- Sun Tzu
Quoted 4 lines · J?rn Engel
Quoted 4 lines
Quoted 4 lines
include/linux/mm.h +0
-0@@ -196,7 +196,7 @@ struct page {#if defined(WANT_PAGE_VIRTUAL )
Quoted 8 lines
Quoted 8 lines
include/linux/mm.h struct page { · line 200
Quoted 5 lines
include/linux/mm.h +1
-1void *virtual;/* Kernel virtual address (NULL if not kmapped, ie. highmem) */ -#endif /* CONFIG_HIGMEM || WANT_PAGE_VIRTUAL */ +#endif /* CONFIG_HIGHMEM || WANT_PAGE_VIRTUAL */ };This is broken, the CONFIG_HIG(H)MEM shouldn't be there at all. Look at the #if defined(...) line.
[snipped]
Looks like this patch made it past Linus's famous patch barriers and into the bk tree after all. J?rn, I trust you'll send a patch to revert it? (Linus, if you want to fix it by hand, just deleted the CONFIG_HIGHMEM bit completely from the #endif line).
Signature · 2 lines
Muli Ben-Yehuda http://www.mulix.org
Quoted 2 lines · Muli Ben-Yehuda
Looks like this patch made it past Linus's famous patch barriers and
Or "infamous"?
I apply obvious typos to comments without really worrying about them too much ;)
Quoted 2 lines
(Linus, if you want to fix it by hand, just deleted the
CONFIG_HIGHMEMbit completely from the #endif line).
Done.
Linus
Quoted 6 lines · Matthew Wilcox
I reviewed the dependency list for a file this morning to see why it was being unnecessarily recompiled (a little fetish of mine, mostly harmless). I was a little discombobulated to find this line:
$(wildcard include/config/higmem.h) \
I was a design decision not to parse comments. Tradeoff between speed and accurateness.
And usually CONFIG_SYMBOLs referred in comments are also used.
The mis-behaviour is not documented in other places than in fixdep itself - and that's not a place I expect people to look anyway. If I at some point in time manage to convince myself to write a how-to-compile-the-kernel I will try to include a section of known shortcoming. This being one of them.
Sam