Message-ID: <Pine.LNX.4.64.0702150736360.20368@woody.linux-foundation.org>
Date:	Thu, 15 Feb 2007 07:57:05 -0800 (PST)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Sergei Organov <osv@...ad.com>
cc:	Pekka Enberg <penberg@...helsinki.fi>,
	J.A. MagallÃÃÃón 
	<jamagallon@....com>, Jan Engelhardt <jengelh@...ux01.gwdg.de>,
	Jeff Garzik <jeff@...zik.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: somebody dropped a (warning) bomb



On Thu, 15 Feb 2007, Sergei Organov wrote:
> 
> Yes, there is. There are at least 2 drawbacks. Minor one is
> initialization eating cycles. Major one is that if later I change the
> code and there will appear a pass that by mistake uses those fake value,
> I won't get the warning anymore. The latter drawback is somewhat similar
> to the drawbacks of explicit type casts.

I actually agree - it would clearly be *nicer* if gcc was accurate with 
the warning. I'm just saying that usually the patch to shut gcc up is 
quite benign.

But yes, it basically disabled the warning, and just *maybe* the warning 
was valid, and it disabled it by initializing something to the wrong 
value. 

That's why warnings with false positives are often much worse than not 
having the warning at all: you start dismissing them, and not even 
bothering to fix them "properly". That's especially true if there _isn't_ 
a proper fix.

I personally much prefer a compiler that doesn't warn a lot, but *when* it 
warns, it's 100% on the money. I think that compilers that warn about 
things that "may" be broken are bogus. It needs to be iron-clad, with no 
question about whether the warning is appropriate!

Which is, of course, why we then shut up some gcc warnings, and which gets 
us back to the start of the discussion.. ;)

> [I'd, personally, try to do code reorganization instead so that it
> becomes obvious for the compiler that the variable can't be used
> uninitialized. Quite often it makes the code better, at least it was my
> experience so far.]

It's true that it sometimes works that way. Not always, though.

The gcc "uninitialized variable" thing is *usually* a good sign that the 
code wasn't straightforward for the compiler to follow, and if the 
compiler cannot follow it, then probably neither can most programmers. So 
together with the fact that it's not at least _syntactically_ ugly to shut 
up, it's not a warning I personally object to most of the time (unlike, 
say, the pointer-sign one ;)

> [Did I already say that I think it's wrong warning to be given in this
> particular case, as the problem with the code is not in signedness?]
> 
> 1. Don't try to hide the warning, at least not immediately, -- consider
>    fixing the code first. Ah, sorry, you've already decided for yourself
>    that the warning is idiotic, so there is no reason to try to...

This is actually something we've tried. 

The problem with that approach is that once you have tons of warnings that 
are "expected", suddenly *all* warnings just become noise. Not just the 
bogus one. It really *is* a matter of: warnings should either be 100% 
solid, or they should not be there at all.

And this is not just about compiler warnings. We've added some warnings of 
our own (well, with compiler help, of course): things like the 
"deprecated" warnings etc. I have often ended up shutting them up, and one 
reason for that is that yes, I have literally added bugs to the kernel 
that the compiler really *did* warn about, but because there were so many 
other pointless warnings, I didn't notice!

I didn't make that up. I forget what the bug was, but it literally wasn't 
more than a couple of months ago that I passed in the wrong type to a 
function, and the compiler warned about it in big letters. It was just 
totally hidden by all the other warning crap.

> 2. If you add a cast, it's not in contrast, it's rather similar to fake
>    initialization above as they have somewhat similar drawback.

I agree that it's "unnecessary code", and in many ways exactly the same 
thing. I just happen to believe that casts tend to be a lot more dangerous 
than extraneous initializations. Casts have this nasty tendency of hiding 
*other* problems too (ie you later change the thing you cast completely, 
and now the compiler doesn't warn about a *real* bug, because the cast 
will just silently force it to a wrong type).

And yeah, that may well be a personal hangup of mine. A lot of people 
think casts in C are normal. Me, I actually consider C to be a type-safe 
language (!) but it's only type-safe if you are careful, and avoiding 
casts is one of the rules.

Others claim that C isn't type-safe at all, and I think it comes from 
them having been involved in projects where people didn't have the same 
"casts are good, but only when you use them really really carefully".

> > But if you have 
> >
> > 	unsigned char *mystring;
> >
> > 	len = strlen(mystring);
> >
> > then please tell me how to fix that warning without making the code 
> > *worse* from a type-safety standpoint? You CANNOT. You'd have to cast it 
> > explicitly (or assing it through a "void *" variable), both of which 
> > actually MAKE THE TYPE-CHECKING PROBLEM WORSE!
> 
> Because instead of trying to fix the code, you insist on hiding the
> warning.

No. I'm saying that I have *reasons* that I need my string to be unsigned. 
That makes your first "fix" totally bogus:

> There are at least two actual fixes:
> 
> 1. Do 'char *mystring' instead, as otherwise compiler can't figure out
>    that you are going to use 'mystring' to point to zero-terminated
>    array of characters.

And the second fix is a fix, but it is, again, worse than the warning:

> 2. Use "len = ustrlen(mystring)" if you indeed need something like C
>    strings

Sure, I can do that, but the upside is what?

Getting rid of a warning that was bogus to begin with?

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

Powered by blists - more mailing lists