jsonb is also breaking the rule against nameless unions

Tom Lane wrote:

Same issue as in
/messages/by-id/31718.1394059256@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 Herrera 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 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

On 2014-04-02 15:17:16 -0300, Alvaro Herrera wrote:

Tom Lane wrote:

Same issue as in
/messages/by-id/31718.1394059256@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

Andres Freund <andres@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

Andres Freund <andres@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

On 2014-04-02 14:36:28 -0400, Tom Lane wrote:

Andres Freund <andres@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

Andres Freund <andres@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

On 2014-04-02 14:42:39 -0400, Tom Lane wrote:

Andres Freund <andres@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

On 2014-04-02 15:03:47 -0400, Tom Lane wrote:

Andres Freund <andres@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

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

On Thu, Apr 3, 2014 at 11:28 AM, Andres Freund <andres@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