Skip to contentSkip to conversation

CONFIG_* In Comments Considered Harmful

PATCH In review Matthew Wilcox 7 participants 11 messages Oct 1, 2003 1 message not in the archive MM core

lore
Share & tools
lore

Matthew Wilcox

author
#0 lore raw

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
diff -u -p -r1.5 mm.h
@@ -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
#1 lore raw
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_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.

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
#2 lore raw
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.

#3 lore raw
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_FOO or CONFIG_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
author
#4 lore raw
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_FOO or CONFIG_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
#5 lore raw
In reply to Dave Jones
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
#6 lore raw
In reply to Matthew Wilcox
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_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
diff -u -p -r1.5 mm.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
diff -u -p -r1.5 mm.h
@@ -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]

#8 lore raw
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.5 mm.h

include/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 -1
 	void *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
#9 lore raw
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 -1
 	void *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
#10 lore raw
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_HIGHMEM bit completely from the #endif line).

Done.

Linus

#11 lore raw
In reply to Matthew Wilcox
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