Re: [HACKERS] jsonb is also breaking the rule against nameless unions
On Thu, Apr 3, 2014 at 11:28 AM, Andres Freund and...@2ndquadrant.com wrote: On 2014-04-02 23:50:19 +0200, Andres Freund wrote: I just tried it on clang. It builds clean with -Wc11-extensions except warning about _Static_assert(). That's possibly fixable with some autoconf trickery. Ah. That sounds promising. What clang version is that? It's debian's clang-3.5, which is built from trunk IIUC: 1:3.5~svn201651-1 I have some patches that fix the configure tests to properly use -Werror, but I am too tired to check their validity now. So, three patches attached: 1) fix a couple of warnings clang outputs in -pedantic. All of them somewhat valid and not too ugly to fix. 2) Use -Werror in a couple more configure checks. That allows to compile pg using both -Wc11-extensions and -Wc99-extensions in a halfway sane fashion. I think this is sane, but I'd welcome comments. 3) Fix C89 compliance issue in pgbench, plus minor cleanups. I don't like the fix here, but I don't really have a better idea. I've committed the first one of these, which looks uncontroversial. The others seem like they might need more discussion, or at least more thought than I can give them right now. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] jsonb is also breaking the rule against nameless unions
On 2014-04-02 23:50:19 +0200, Andres Freund wrote:
I just tried it on clang. It builds clean with -Wc11-extensions except
warning about _Static_assert(). That's possibly fixable with some
autoconf trickery.
Ah. That sounds promising. What clang version is that?
It's debian's clang-3.5, which is built from trunk IIUC: 1:3.5~svn201651-1
I have some patches that fix the configure tests to properly use
-Werror, but I am too tired to check their validity now.
So, three patches attached:
1) fix a couple of warnings clang outputs in -pedantic. All of them
somewhat valid and not too ugly to fix.
2) Use -Werror in a couple more configure checks. That allows to compile
pg using both -Wc11-extensions and -Wc99-extensions in a halfway sane
fashion. I think this is sane, but I'd welcome comments.
3) Fix C89 compliance issue in pgbench, plus minor cleanups. I don't
like the fix here, but I don't really have a better idea.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
From 3219b14205f33e4e6c3682cf9ca795159d59e611 Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Thu, 3 Apr 2014 12:51:40 +0200
Subject: [PATCH 1/3] Fix a bunch of somewhat pedantic compiler warnings.
In C89 it's not allowed to have a trailing comma after the last
enumerator value, compound literals have to be compile time constant
and strictly speaking a void * pointer isn't a function pointer...
---
src/backend/access/transam/xlog.c | 2 +-
src/bin/pg_dump/parallel.c | 5 -
src/bin/psql/tab-complete.c| 10 +-
src/include/catalog/objectaccess.h | 2 +-
src/include/pgstat.h | 2 +-
src/include/utils/jsonapi.h| 2 +-
6 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f9d6609..5096999 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -651,7 +651,7 @@ typedef enum
XLOG_FROM_ANY = 0, /* request to read WAL from any source */
XLOG_FROM_ARCHIVE, /* restored using restore_command */
XLOG_FROM_PG_XLOG, /* existing file in pg_xlog */
- XLOG_FROM_STREAM, /* streamed from master */
+ XLOG_FROM_STREAM /* streamed from master */
} XLogSource;
/* human-readable names for XLogSources, for debugging output */
diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index 6f2634b..7d6e146 100644
--- a/src/bin/pg_dump/parallel.c
+++ b/src/bin/pg_dump/parallel.c
@@ -558,7 +558,10 @@ ParallelBackupStart(ArchiveHandle *AH, RestoreOptions *ropt)
{
/* we are the worker */
int j;
- int pipefd[2] = {pipeMW[PIPE_READ], pipeWM[PIPE_WRITE]};
+ int pipefd[2];
+
+ pipefd[0] = pipeMW[PIPE_READ];
+ pipefd[1] = pipeWM[PIPE_WRITE];
/*
* Store the fds for the reverse communication in pstate. Actually
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 1d69b95..202ffce 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -781,7 +781,7 @@ static const pgsql_thing_t words_after_create[] = {
/* Forward declaration of functions */
-static char **psql_completion(char *text, int start, int end);
+static char **psql_completion(const char *text, int start, int end);
static char *create_command_generator(const char *text, int state);
static char *drop_command_generator(const char *text, int state);
static char *complete_from_query(const char *text, int state);
@@ -790,7 +790,7 @@ static char *_complete_from_query(int is_schema_query,
const char *text, int state);
static char *complete_from_list(const char *text, int state);
static char *complete_from_const(const char *text, int state);
-static char **complete_from_variables(char *text,
+static char **complete_from_variables(const char *text,
const char *prefix, const char *suffix);
static char *complete_from_files(const char *text, int state);
@@ -812,7 +812,7 @@ void
initialize_readline(void)
{
rl_readline_name = (char *) pset.progname;
- rl_attempted_completion_function = (void *) psql_completion;
+ rl_attempted_completion_function = psql_completion;
rl_basic_word_break_characters = WORD_BREAKS;
@@ -834,7 +834,7 @@ initialize_readline(void)
* completion_matches() function, so we don't have to worry about it.
*/
static char **
-psql_completion(char *text, int start, int end)
+psql_completion(const char *text, int start, int end)
{
/* This is the variable we'll return. */
char **matches = NULL;
@@ -3847,7 +3847,7 @@ complete_from_const(const char *text, int state)
* to support quoting usages.
*/
static char **
-complete_from_variables(char *text, const char *prefix, const char *suffix)
+complete_from_variables(const char *text, const char *prefix, const char *suffix)
{
char **matches;
char **varnames;
diff --git
Re: [HACKERS] jsonb is also breaking the rule against nameless unions
Tom Lane wrote: Same issue as in http://www.postgresql.org/message-id/31718.1394059...@sss.pgh.pa.us In file included from jsonb.c:19: ../../../../src/include/utils/jsonb.h:195: warning: unnamed struct/union that defines no instances jsonb.c: In function `jsonb_in_object_field_start': jsonb.c:250: structure has no member named `string' jsonb.c:251: structure has no member named `string' jsonb.c:251: structure has no member named `string' jsonb.c:252: structure has no member named `string' jsonb.c: In function `jsonb_put_escaped_value': jsonb.c:266: structure has no member named `string' jsonb.c:266: structure has no member named `string' jsonb.c:271: structure has no member named `numeric' jsonb.c:274: structure has no member named `boolean' jsonb.c: In function `jsonb_in_scalar': jsonb.c:301: structure has no member named `string' jsonb.c:302: structure has no member named `string' jsonb.c:302: structure has no member named `string' jsonb.c:303: structure has no member named `string' ... etc etc etc ... We really need to get a buildfarm member going that complains about this. I had hoped to install a sufficiently old gcc version on prairiedog or dromedary, but didn't have much luck rebuilding ancient gcc releases on OS X. Complain how? I find that gcc -std=c90 -pedantic emits these warnings about it: def.c:3:24: warning: ISO C90 doesn’t support unnamed structs/unions [-pedantic] def.c:1:8: warning: struct has no named members [-pedantic] -- Álvaro Herrerahttp://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
Re: [HACKERS] jsonb is also breaking the rule against nameless unions
On 2014-04-02 13:56:40 -0400, Tom Lane wrote: We really need to get a buildfarm member going that complains about this. I had hoped to install a sufficiently old gcc version on prairiedog or dromedary, but didn't have much luck rebuilding ancient gcc releases on OS X. Some experimentation shows that clang's -Wc11-extensions warns about this... If we could get the build on clang warnings free we could use that together with -Werror... 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
Re: [HACKERS] jsonb is also breaking the rule against nameless unions
On 2014-04-02 15:17:16 -0300, Alvaro Herrera wrote: Tom Lane wrote: Same issue as in http://www.postgresql.org/message-id/31718.1394059...@sss.pgh.pa.us In file included from jsonb.c:19: ../../../../src/include/utils/jsonb.h:195: warning: unnamed struct/union that defines no instances jsonb.c: In function `jsonb_in_object_field_start': jsonb.c:250: structure has no member named `string' jsonb.c:251: structure has no member named `string' jsonb.c:251: structure has no member named `string' jsonb.c:252: structure has no member named `string' jsonb.c: In function `jsonb_put_escaped_value': jsonb.c:266: structure has no member named `string' jsonb.c:266: structure has no member named `string' jsonb.c:271: structure has no member named `numeric' jsonb.c:274: structure has no member named `boolean' jsonb.c: In function `jsonb_in_scalar': jsonb.c:301: structure has no member named `string' jsonb.c:302: structure has no member named `string' jsonb.c:302: structure has no member named `string' jsonb.c:303: structure has no member named `string' ... etc etc etc ... We really need to get a buildfarm member going that complains about this. I had hoped to install a sufficiently old gcc version on prairiedog or dromedary, but didn't have much luck rebuilding ancient gcc releases on OS X. Complain how? I find that gcc -std=c90 -pedantic emits these warnings about it: def.c:3:24: warning: ISO C90 doesn’t support unnamed structs/unions [-pedantic] def.c:1:8: warning: struct has no named members [-pedantic] Last time I checked gcc builds of postgres using -pedantic are so verbose that warnings don't have an effect anymore. Is that not the case anymore? 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
Re: [HACKERS] jsonb is also breaking the rule against nameless unions
Andres Freund and...@2ndquadrant.com writes: On 2014-04-02 15:17:16 -0300, Alvaro Herrera wrote: Tom Lane wrote: We really need to get a buildfarm member going that complains about this. Complain how? I find that gcc -std=c90 -pedantic emits these warnings about it: def.c:3:24: warning: ISO C90 doesnât support unnamed structs/unions [-pedantic] def.c:1:8: warning: struct has no named members [-pedantic] Last time I checked gcc builds of postgres using -pedantic are so verbose that warnings don't have an effect anymore. Is that not the case anymore? Well, in any case, people very seldom check to see if any buildfarm members are producing compiler warnings. You need the build to actually go red to get anyone's attention reliably. I concur that -pedantic is pretty much useless for our purposes anyway. The non-C89 feature that I've been really worried about is flexible array members (which we intend to start using more heavily, so we need a complaint if someone leaves out the FLEXIBLE_ARRAY_MEMBER macro). Based on the last month or so I guess that anonymous unions are a big issue as well. I'd like to have a buildfarm member whose compiler doesn't recognize either of those ... and AFAICT, -pedantic is no help for the array case. 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
Re: [HACKERS] jsonb is also breaking the rule against nameless unions
On 2014-04-02 14:36:28 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2014-04-02 15:17:16 -0300, Alvaro Herrera wrote: Tom Lane wrote: We really need to get a buildfarm member going that complains about this. Complain how? I find that gcc -std=c90 -pedantic emits these warnings about it: def.c:3:24: warning: ISO C90 doesn’t support unnamed structs/unions [-pedantic] def.c:1:8: warning: struct has no named members [-pedantic] Last time I checked gcc builds of postgres using -pedantic are so verbose that warnings don't have an effect anymore. Is that not the case anymore? Well, in any case, people very seldom check to see if any buildfarm members are producing compiler warnings. You need the build to actually go red to get anyone's attention reliably. Yea, we'd need to be able to turn on -Werror if it's going to have any effect. I don't think our configure currently copes with that unfortunately... I just tried it on clang. It builds clean with -Wc11-extensions except warning about _Static_assert(). That's possibly fixable with some autoconf trickery. The non-C89 feature that I've been really worried about is flexible array members (which we intend to start using more heavily, so we need a complaint if someone leaves out the FLEXIBLE_ARRAY_MEMBER macro). Based on the last month or so I guess that anonymous unions are a big issue as well. I'd like to have a buildfarm member whose compiler doesn't recognize either of those ... and AFAICT, -pedantic is no help for the array case. gcc's -pedantic warns about flexible array members here, but it doesn't solve the problem with it being unusable :( 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
Re: [HACKERS] jsonb is also breaking the rule against nameless unions
Andres Freund and...@2ndquadrant.com writes: On 2014-04-02 13:56:40 -0400, Tom Lane wrote: We really need to get a buildfarm member going that complains about this. I had hoped to install a sufficiently old gcc version on prairiedog or dromedary, but didn't have much luck rebuilding ancient gcc releases on OS X. Some experimentation shows that clang's -Wc11-extensions warns about this... If we could get the build on clang warnings free we could use that together with -Werror... What's it warning about currently? 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
Re: [HACKERS] jsonb is also breaking the rule against nameless unions
Andres Freund and...@2ndquadrant.com writes: On 2014-04-02 14:36:28 -0400, Tom Lane wrote: Well, in any case, people very seldom check to see if any buildfarm members are producing compiler warnings. You need the build to actually go red to get anyone's attention reliably. Yea, we'd need to be able to turn on -Werror if it's going to have any effect. I don't think our configure currently copes with that unfortunately... I'm pretty sure you can set CFLAGS from the buildfarm configuration options --- see the animals that are building with -DCLOBBER_CACHE_ALWAYS. I just tried it on clang. It builds clean with -Wc11-extensions except warning about _Static_assert(). That's possibly fixable with some autoconf trickery. Ah. That sounds promising. What clang version is that? 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
Re: [HACKERS] jsonb is also breaking the rule against nameless unions
On 2014-04-02 14:42:39 -0400, Tom Lane wrote:
Andres Freund and...@2ndquadrant.com writes:
On 2014-04-02 13:56:40 -0400, Tom Lane wrote:
We really need to get a buildfarm member going that complains about this.
I had hoped to install a sufficiently old gcc version on prairiedog or
dromedary, but didn't have much luck rebuilding ancient gcc releases on
OS X.
Some experimentation shows that clang's -Wc11-extensions warns about
this... If we could get the build on clang warnings free we could use
that together with -Werror...
What's it warning about currently?
So, when I manually put a #undef HAVE__STATIC_ASSERT somewhere relevant,
I can compile pg warning free under clang trunk with:
-std=c89 -Wall -Wextra -pedantic
-Wc11-extensions -Wmissing-declarations
-Wno-unused-parameter -Wno-sign-compare -Wno-missing-field-initializers
-Wno-overlength-strings -Wno-variadic-macros -Wno-long-long
-Wno-gnu-statement-expression
without any warnings.
If I add -Wc99-extensions and remove -pedantic it complains about
FLEXIBLE_ARRAY_MEMBER, commas at the end of enumerator lists, extended
field designators (e.g. offsetof(POLYGON, p[0])).
That it warns about FLEXIBLE_ARRAY_MEMBER suggest our configure test for
that could use some improvement. Commas at the enum of enum list are
easily fixed (patch attached).
The extended offsetof bit is a bit more critical, we use that pretty
widely. Luckily it can be disabled with -Wno-extended-offsetof
There's also the valid warning about:
/home/andres/src/postgresql/src/bin/pg_dump/parallel.c:561:22: warning:
initializer for aggregate is not a compile-time constant
[-Wc99-extensions]
int pipefd[2] =
{pipeMW[PIPE_READ], pipeWM[PIPE_WRITE]};
^~~
1 warning generated.
pedantic also complains about:
/home/andres/src/postgresql/src/bin/psql/tab-complete.c:815:35: warning:
assigning to 'rl_completion_func_t *'
(aka 'char **(*)(const char *, int, int)') from 'void *' converts
between void pointer and function pointer [-Wpedantic]
rl_attempted_completion_function = (void *) psql_completion;
^
which seems valid and solvable.
But there's also:
preproc.y:15039:7: warning: C requires #line number to be less than
32768, allowed as extension [-Wpedantic]
#line 51524 preproc.c
^
which seems pretty pointless.
Some thoughts about -Wno-* flags needed:
-Wno-overlength-strings: I don't care.
-Wno-variadic-macros: suggests the configure test is missing a step.
-Wno-long-long: Looks like sloppy coding in seldomly looked at parts to me.
-Wno-gnu-statement-expression: Haven't looked
This doesn't look too bad. At least without -pedantic the warnings seem
sensible after disabling an option or two...
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
Re: [HACKERS] jsonb is also breaking the rule against nameless unions
On 2014-04-02 15:03:47 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2014-04-02 14:36:28 -0400, Tom Lane wrote: Well, in any case, people very seldom check to see if any buildfarm members are producing compiler warnings. You need the build to actually go red to get anyone's attention reliably. Yea, we'd need to be able to turn on -Werror if it's going to have any effect. I don't think our configure currently copes with that unfortunately... I'm pretty sure you can set CFLAGS from the buildfarm configuration options --- see the animals that are building with -DCLOBBER_CACHE_ALWAYS. The problem is rather that configure dies somewhere when CFLAGS includes -Werror. Not very nice imo. I just tried it on clang. It builds clean with -Wc11-extensions except warning about _Static_assert(). That's possibly fixable with some autoconf trickery. Ah. That sounds promising. What clang version is that? It's debian's clang-3.5, which is built from trunk IIUC: 1:3.5~svn201651-1 I have some patches that fix the configure tests to properly use -Werror, but I am too tired to check their validity now. 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
