Re: jsonb is also breaking the rule against nameless unions

Same issue as in
http://www.postgresql.org/message-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.

regards, tom lane

Tom Lane wrote:
> Same issue as in
> http://www.postgresql.org/message-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

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

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.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

Andres Freund <andres(at)2ndquadrant(dot)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 doesnt 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

Andres Freund <andres(at)2ndquadrant(dot)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

On 2014-04-02 14:36:28 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)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

Andres Freund <andres(at)2ndquadrant(dot)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

On 2014-04-02 14:42:39 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)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

On 2014-04-02 15:03:47 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)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

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(at)2ndquadrant(dot)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