[PATCH] Compile without warning with gcc's -Wtype-limits, -Wempty-body
Hi,
the attached trivial patch allows to compile with -Wtype-limits
-Wempty-body (or -Wextra -Wno-unused-parameter -Wno-sign-compare
-Wno-missing-field-initializers).
As the two fixes seem harmless, that seems to be a good idea. And the
recent "bug" (its not really that, but ...) in walsender.c shows that at
least -Wtype-limits is a sensible warning.
Independently from this patch, should we add -Wtype-limits to the
default parameters?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
no-warnings-with-W-type-limits.patchtext/x-patch; charset=us-asciiDownload
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index f2cf5c6..263fe5a 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -331,8 +331,8 @@ PROCLOCK_PRINT(const char *where, const PROCLOCK *proclockP)
}
#else /* not LOCK_DEBUG */
-#define LOCK_PRINT(where, lock, type)
-#define PROCLOCK_PRINT(where, proclockP)
+#define LOCK_PRINT(where, lock, type) (void)0
+#define PROCLOCK_PRINT(where, proclockP) (void)0
#endif /* not LOCK_DEBUG */
diff --git a/src/backend/tsearch/wparser_def.c b/src/backend/tsearch/wparser_def.c
index cb2f8eb..d8c6be5 100644
--- a/src/backend/tsearch/wparser_def.c
+++ b/src/backend/tsearch/wparser_def.c
@@ -1811,7 +1811,7 @@ TParserGet(TParser *prs)
pg_mblen(prs->str + prs->state->posbyte);
Assert(prs->state->posbyte + prs->state->charlen <= prs->lenstr);
- Assert(prs->state->state >= TPS_Base && prs->state->state < TPS_Null);
+ Assert(prs->state->state < TPS_Null);
Assert(Actions[prs->state->state].state == prs->state->state);
if (prs->state->pushedAtAction)
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.
FWIW, clang has the same warning on by default. There, it's called
-Wtautological-compare.
I'm less concerned about -Wempty-body, but I can't see the practical use
either way.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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...
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.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.
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.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-01-14 22:26:39 -0500, Tom Lane wrote:
Andres Freund <andres@2ndquadrant.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 1/15/13 6:36 AM, Andres Freund wrote:
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.
Well, once you get past that one change you proposed, you will also find
pg_standby.c: In function 'SetWALFileNameForCleanup':
pg_standby.c:348:3: error: comparison of unsigned expression >= 0 is
always true [-Werror=type-limits]
(which, curiously, is the only one that clang complains about).
I don't like removing safety checks from code when there is no other
mechanism that could make up for it somehow.
I think the best practice at the moment, as with most gcc -Wextra
warnings, is to manually check them once in a while.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers