Re: compile time warnings

[Jesper Juhl]

On Fri, 2 Jan 2009, Ingo Molnar wrote:
> 
> * Jesper Juhl <jj@xxxxxxxxxxxxx> wrote:
> 
> > On Fri, 2 Jan 2009, Tom Spink wrote:
> > 
> > > 2009/1/1 Jesper Juhl <jj@xxxxxxxxxxxxx>:
> > > > On Thu, 1 Jan 2009, Ingo Brueckl wrote:
> > > [snip]
> > > 
> > > Hi,
> > > 
> > > > pgd_base is very much used...
> > > 
> > > It's probably something to do with:
> > > 
> > > # define permanent_kmaps_init(pgd_base)         do { } while (0)
> > > 
> > > Which is within the #else part of #if CONFIG_HIGHMEM.  So, if
> > > CONFIG_HIGHMEM is not set, permanent_kmaps_init gets wiped out, and
> > > therefore that warning will be issued.
> > > 
> > > Perhaps changing that to an empty inline would remove the warning?
> > > 
> > Yeah, I noticed that as well after sending the mail.
> > Another way to silence the warning (which I think is nicer) would be 
> > something like this;
> > 
> > 
> > Silence 'unused variable' warning in arch/x86/mm/init_32.c::pagetable_init
> > 
> > Signed-off-by: Jesper Juhl <jj@xxxxxxxxxxxxx>
> > ---
> > 
> > diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
> > index 8655b5b..0affa8e 100644
> > --- a/arch/x86/mm/init_32.c
> > +++ b/arch/x86/mm/init_32.c
> > @@ -511,9 +511,7 @@ static void __init early_ioremap_page_table_range_init(pgd_t *pgd_base)
> >  
> >  static void __init pagetable_init(void)
> >  {
> > -	pgd_t *pgd_base = swapper_pg_dir;
> > -
> > -	permanent_kmaps_init(pgd_base);
> > +	permanent_kmaps_init((pgd_t *)swapper_pg_dir);
> >  }
> 
> no, this only works around the warning by 'silencing' it (it also includes 
> an ugly type cast) - instead of fixing the core problem.
> 
Hmm, true..

> The core problem is that permanent_kmaps_init() is a CPP macro in the 
> !HIGHMEM case - so the right fix would be to convert that to a proper C 
> inline function. (same for set_highmem_pages_init() while at it)
> 
> Would you mind to send a patch for that that we could push to Linus?
>
Sure, I'll do that.
 
> <soapbox>
> 
> The highest quality fixes that are motivated by compiler warnings are the 
> ones that do not actually 'fix a warning', but instead improve/reshape 
> some code so that as a side-effect the warning goes away.
> 
> If you ever see a patch that 'silences a warning', it usually shows that 
> the deeper problem has not been fully understood. (Except of course if the 
> warning shows a genuine bug in the code - but in that case we fix the bug 
> and not the warning - the warning was just the canary to it.)
> 
> </soapbox>
> 
Well said.

> 	Ingo
> 

-- 
Jesper Juhl <jj@xxxxxxxxxxxxx>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/