[PATCH] CONFIG_* In Comments Considered Harmful

[PATCH] CONFIG_* In Comments Considered Harmful

[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
--- a/include/linux/mm.h	28 Sep 2003 04:06:20 -0000	1.5
+++ b/include/linux/mm.h	1 Oct 2003 13:15:53 -0000
@@ -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
 };
 
 /*

-- 
"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

Re: [PATCH] CONFIG_* In Comments Considered Harmful

[Dave Jones]

On Wed, Oct 01, 2003 at 02:26:19PM +0100, Matthew Wilcox wrote:
 > 
 > 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.

 >     $(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

-- 
 Dave Jones     http://www.codemonkey.org.uk

Re: [PATCH] CONFIG_* In Comments Considered Harmful

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 344 bytes --]

On Wed, 01 Oct 2003 15:19:52 BST, Dave Jones said:

> 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.

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] CONFIG_* In Comments Considered Harmful

[Jörn Engel]

On Wed, 1 October 2003 10:29:55 -0400, Valdis.Kletnieks@vt.edu wrote:
> On Wed, 01 Oct 2003 15:19:52 BST, Dave Jones said:
> 
> > 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.

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

-- 
The strong give up and move away, while the weak give up and stay.
-- unknown

Re: [PATCH] CONFIG_* In Comments Considered Harmful

[Matthew Wilcox]

On Wed, Oct 01, 2003 at 04:42:23PM +0200, Jörn Engel wrote:
> On Wed, 1 October 2003 10:29:55 -0400, Valdis.Kletnieks@vt.edu wrote:
> > On Wed, 01 Oct 2003 15:19:52 BST, Dave Jones said:
> > 
> > > 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.
> 
> 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;

-- 
"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

Re: [PATCH] CONFIG_* In Comments Considered Harmful

[Muli Ben-Yehuda]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

On Wed, Oct 01, 2003 at 03:19:52PM +0100, Dave Jones wrote:

> 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. 
-- 
Muli Ben-Yehuda
http://www.mulix.org


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] CONFIG_* In Comments Considered Harmful

[Jörn Engel]

On Wed, 1 October 2003 14:26:19 +0100, Matthew Wilcox wrote:
> 
> 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
> --- a/include/linux/mm.h	28 Sep 2003 04:06:20 -0000	1.5
> +++ b/include/linux/mm.h	1 Oct 2003 13:15:53 -0000
> @@ -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
diff -u -p -r1.5 mm.h
--- a/include/linux/mm.h	28 Sep 2003 04:06:20 -0000	1.5
+++ b/include/linux/mm.h	1 Oct 2003 13:15:53 -0000
@@ -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

-- 
The only real mistake is the one from which we learn nothing.
-- John Powell

Re: [PATCH] CONFIG_* In Comments Considered Harmful

[Jörn Engel]

On Wed, 1 October 2003 17:52:06 +0300, Muli Ben-Yehuda wrote:
> > 
> > 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
> > --- a/include/linux/mm.h	28 Sep 2003 04:06:20 -0000	1.5
> > +++ b/include/linux/mm.h	1 Oct 2003 13:15:53 -0000
> > @@ -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 */
> >  };
> 
> 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

-- 
A defeated army first battles and then seeks victory.
-- Sun Tzu

Re: [PATCH] CONFIG_* In Comments Considered Harmful

[Muli Ben-Yehuda]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

On Wed, Oct 01, 2003 at 05:10:11PM +0200, J?rn Engel wrote:

> > > --- a/include/linux/mm.h	28 Sep 2003 04:06:20 -0000	1.5
> > > +++ b/include/linux/mm.h	1 Oct 2003 13:15:53 -0000
> > > @@ -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 */
> > >  };
> > 
> > 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).
-- 
Muli Ben-Yehuda
http://www.mulix.org


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] CONFIG_* In Comments Considered Harmful

[Linus Torvalds]

On Wed, 1 Oct 2003, Muli Ben-Yehuda wrote:
> 
> 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 ;)

> (Linus, if you want to fix it by hand, just deleted the
> CONFIG_HIGHMEM bit completely from the #endif line).

Done.

		Linus

Re: [PATCH] CONFIG_* In Comments Considered Harmful

[Sam Ravnborg]

On Wed, Oct 01, 2003 at 02:26:19PM +0100, Matthew Wilcox wrote:
> 
> 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