Обсуждение: 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@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
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
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
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
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
voidpointer 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@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
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