Re: [PATCH] Compile without warning with gcc's -Wtype-limits, -Wempty-body

On 2013-01-14 22:26:39 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2013-01-14 20:39:05 -0500, Peter Eisentraut wrote:
> >> On Tue, 2013-01-15 at 00:29 +0100, Andres Freund wrote:
> >>> Independently from this patch, should we add -Wtype-limits to the
> >>> default parameters?
>
> >> I think we have had a discussion along this line before. I am against
> >> fixing warnings from this option, because those changes would hide
> >> errors if a variable's type changed from signed to unsigned or vice
> >> versa, which could happen because of refactoring or it might be
> >> dependent on system headers.
>
> > Well, I already found a bug (although with very limited consequences) in
> > the walsender code and one with graver consequences in code I just
> > submitted. So I don't really see that being on-par with some potential
> > future refactoring...
>
> FWIW, I agree with Peter --- in particular, warning against "x >= MIN"
> just because MIN happens to be zero and x happens to be unsigned is the
> sort of nonsense up with which we should not put. Kowtowing to that
> kind of warning makes the code less robust, not more so.

Oh, I agree, that warning is pointless in itself.

But in general doing a comparison like > 0 *can* show a programming
error as evidenced e.g. by
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=3f4b1749a8168893558f70021be4f40c650bbada
and just about the same error I made in xlogdump.

I just think that the price of fixing a single Assert() that hasn't
changed in years where the variable isn't likely to ever get signed is
acceptable.

> It's a shame that the compiler writers have not figured this out and
> separated misguided pedantry from actually-useful warnings. If I assign
> -1 to an unsigned variable, by all means tell me about *that*. Don't
> tell me your opinion of whether an assertion check is necessary.

Yea, but I have to admit its damned hard hard to automatically discern
the above actually valid warning and the bogus Assert...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services