Re: [HACKERS] jsonb is also breaking the rule against nameless unions
On Thu, Apr 3, 2014 at 11:28 AM, Andres Freund 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
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 a/src/include/ca
Re: [HACKERS] jsonb is also breaking the rule against nameless unions
On 2014-04-02 15:03:47 -0400, Tom Lane wrote: > Andres Freund 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
Re: [HACKERS] jsonb is also breaking the rule against nameless unions
On 2014-04-02 14:42:39 -0400, Tom Lane wrote:
> Andres Freund 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
Andres Freund 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
Andres Freund 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
On 2014-04-02 14:36:28 -0400, Tom Lane wrote: > Andres Freund 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 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 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
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
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
[HACKERS] jsonb is also breaking the rule against nameless unions
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. 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
