Обсуждение: Further news on Clang - spurious warnings

I'm happy to report that thanks to some persistent complaining on my
part, the one outstanding issue when building Postgres with Clang -
the spurious warnings that occured as a result of it being statically
detected that there are assignments past what appears to be the end of
a single element array at the end of a struct - has been fixed in a
recent revision. Now, the only warning that remains is that same,
annoying Flex related bug also seen with GCC that we can't seem to do
anything about, because the Flex people refuse to acknowledge that
it's a bug.

However, at least when you see this warning when using Clang, you get
to see a comment beside the declaration that hints that the warning is
spurious:

[peter@laptop postgresql]$ /home/peter/build/Release/bin/clang -O2
-Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wformat-security
-fno-strict-aliasing -fwrapv -Wno-error -I. -I. -Isrc/include
-D_GNU_SOURCE   -c -o gram.o src/backend/parser/gram.c
In file included from gram.y:12949:
scan.c:16246:23: warning: unused variable 'yyg' [-Wunused-variable]   struct yyguts_t * yyg = (struct
yyguts_t*)yyscanner;/* This var 
may be unused depending upon options. */                     ^
1 warning generated.


This is not the case with GCC 4.6:

[peter@laptop postgresql]$ gcc -O2 -Wall -Wmissing-prototypes
-Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wformat-security -fno-strict-aliasing -fwrapv -Wno-error -I. -I.
-Isrc/include -D_GNU_SOURCE   -c -o gram.o src/backend/parser/gram.c
In file included from gram.y:12949:0:
scan.c: In function ‘yy_try_NUL_trans’:
scan.c:16246:23: warning: unused variable ‘yyg’ [-Wunused-variable]

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On 3 August 2011 00:52, Peter Geoghegan <peter@2ndquadrant.com> wrote:
> Now, the only warning that remains is that same

Correction - there are actually 3 additional warnings like this in repl_gram.c:

/home/peter/build/Release/bin/clang -O2 -Wall -Wmissing-prototypes
-Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wformat-security -fno-strict-aliasing -fwrapv -I.
-I../../../src/include -D_GNU_SOURCE   -c -o repl_gram.o repl_gram.c
repl_gram.y:106:30: warning: implicit conversion from enumeration type
'enum ReplNodeTag' to different enumeration type 'NodeTag' (aka 'enum
NodeTag') [-Wconversion]                                       (yyval.node) = (Node *)
makeNode(IdentifySystemCmd);

^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../src/include/nodes/nodes.h:475:64: note: expanded from:
#define makeNode(_type_)                ((_type_ *)
newNode(sizeof(_type_),T_##_type_))                                                                            ^
<scratch space>:180:1: note: expanded from:
T_IdentifySystemCmd
^
../../../src/include/nodes/nodes.h:452:19: note: expanded from:       _result->type = (tag); \                     ~
^~~

I'll take a look into them later.

This tautology warning sounds completely valid:

/home/peter/build/Release/bin/clang -O2 -Wall -Wmissing-prototypes
-Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wformat-security -fno-strict-aliasing -fwrapv -pthread  -D_REENTRANT
-D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS -fpic -DFRONTEND
-DUNSAFE_STAT_OK -I. -I../../../src/include -D_GNU_SOURCE
-I../../../src/port -I../../../src/port -DSO_MAJOR_VERSION=5  -c -o
fe-exec.o fe-exec.c
fe-exec.c:2408:13: warning: comparison of unsigned enum expression < 0
is always false [-Wtautological-compare]       if (status < 0 || status >= sizeof pgresStatus / sizeof pgresStatus[0])
        ~~~~~~ ^ ~ 
1 warning generated.

So, there are 4 remaining warnings.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

Attached patch removes the tautologolical part of an evaluated
expression, fixing the problem flagged by this quite valid warning.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On 03.08.2011 12:25, Peter Geoghegan wrote:
> Attached patch removes the tautologolical part of an evaluated
> expression, fixing the problem flagged by this quite valid warning.

The check is only tautological if the compiler implements enums as 
unsigned integers. Whether enums are signed or not is 
implementation-dependent. Perhaps cast status to unsigned or signed 
explicitly before the checks?

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com

On 3 August 2011 10:34, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
> The check is only tautological if the compiler implements enums as unsigned
> integers. Whether enums are signed or not is implementation-dependent.
> Perhaps cast status to unsigned or signed explicitly before the checks?

I don't believe that the standard allows for an implementation of
enums as unsigned integers - after all, individual enum literals can
be given corresponding negative integer values.

This warning is only seen because the first enum literal in the enum
is explicitly given the value 0, thus precluding the possibility of
the value being < 0, barring some abuse of the enum.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On 03.08.2011 13:05, Peter Geoghegan wrote:
> I don't believe that the standard allows for an implementation of
> enums as unsigned integers - after all, individual enum literals can
> be given corresponding negative integer values.

C99 says:

> Each enumerated type shall be compatible with char, a signed integer type, or an
> unsigned integer type. The choice of type is implementation-defined,110) but shall be
> capable of representing the values of all the members of the enumeration.

See also:

http://stackoverflow.com/questions/2579230/signedness-of-enum-in-c-c99-c-cx-gnu-c-gnu-c99

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com

On 3 August 2011 11:05, Peter Geoghegan <peter@2ndquadrant.com> wrote:
> I don't believe that the standard allows for an implementation of
> enums as unsigned integers - after all, individual enum literals can
> be given corresponding negative integer values.

It actually gives leeway to implement the enum as unsigned int when
the compiler knows that it won't matter, because there are no negative
integer values that correspond to some enum literal. The hint was in
my original warning.  :-)

> This warning is only seen because the first enum literal in the enum
> is explicitly given the value 0, thus precluding the possibility of
> the value being < 0, barring some abuse of the enum.

It's also seen if no explicit values are given, and the compiler opts
to make the representation unsigned. It is not seen if it the value is
-1, for example.

Despite the fact that whether or not the value is unsigned is
implementation defined, I think that the patch is still valid - the
expression is at least logically tautological, even if it isn't
necessarily bitwise tautological, because, as I said, barring some
violation of the enum's contract, it should not be < 0. That's
precisely why the compiler has opted to make it unsigned.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On 03.08.2011 14:13, Peter Geoghegan wrote:
> On 3 August 2011 11:05, Peter Geoghegan<peter@2ndquadrant.com>  wrote:
>> I don't believe that the standard allows for an implementation of
>> enums as unsigned integers - after all, individual enum literals can
>> be given corresponding negative integer values.
>
> It actually gives leeway to implement the enum as unsigned int when
> the compiler knows that it won't matter, because there are no negative
> integer values that correspond to some enum literal. The hint was in
> my original warning.  :-)
>
>> This warning is only seen because the first enum literal in the enum
>> is explicitly given the value 0, thus precluding the possibility of
>> the value being<  0, barring some abuse of the enum.
>
> It's also seen if no explicit values are given, and the compiler opts
> to make the representation unsigned. It is not seen if it the value is
> -1, for example.
>
> Despite the fact that whether or not the value is unsigned is
> implementation defined, I think that the patch is still valid - the
> expression is at least logically tautological, even if it isn't
> necessarily bitwise tautological, because, as I said, barring some
> violation of the enum's contract, it should not be<  0. That's
> precisely why the compiler has opted to make it unsigned.

Right, but the purpose of that check is to defend from programmer error. 
If the programmer screws up and calls "PQresStatus(-1)", we want to give 
an error, not crash. If you assume that the programmer will only pass a 
valid enum constant as parameter, then you might as well remove the 
if-statement altogether. I don't think that would be an improvement.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com

On 3 August 2011 12:19, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

> Right, but the purpose of that check is to defend from programmer error. If
> the programmer screws up and calls "PQresStatus(-1)", we want to give an
> error, not crash. If you assume that the programmer will only pass a valid
> enum constant as parameter, then you might as well remove the if-statement
> altogether. I don't think that would be an improvement.

Ahh. I failed to consider the intent of the code.

Attached patch has a better solution than casting though - we simply
use an enum literal. The fact that PGRES_EMPTY_QUERY is the first
value in the enum can be safely assumed to be stable, not least
because we've even already explicitly given it a corresponding value
of 0.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

It occurred to me that you may be a little surprised that the patch
actually prevents the warning from occurring. If you have any doubts,
I can assure you that it does. Clang differentiates between 0 as an
rvalue, 0 from an enum literal and 0 resulting from evaluating a macro
expression (including a function-like macro with deep nesting).

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

Peter Geoghegan <peter@2ndquadrant.com> writes:
> On 3 August 2011 12:19, Heikki Linnakangas
> <heikki.linnakangas@enterprisedb.com> wrote:
>> Right, but the purpose of that check is to defend from programmer error. If
>> the programmer screws up and calls "PQresStatus(-1)", we want to give an
>> error, not crash. If you assume that the programmer will only pass a valid
>> enum constant as parameter, then you might as well remove the if-statement
>> altogether. I don't think that would be an improvement.

> Ahh. I failed to consider the intent of the code.

> Attached patch has a better solution than casting though - we simply
> use an enum literal. The fact that PGRES_EMPTY_QUERY is the first
> value in the enum can be safely assumed to be stable, not least
> because we've even already explicitly given it a corresponding value
> of 0.

No, this is not an improvement at all.  The point of the code is that we
are about to use the enum value as an integer array subscript, and we
want to make sure it is within the array bounds.  Tying that logic to
some member of the enum is not a readability or safety improvement.
We aren't trusting the caller to pass a valid enum value, and likewise
this code doesn't particularly want to trust the enum definition to be
anything in particular.

There is another point here, though, which is that if we're not sure
whether the compiler considers ExecStatusType to be signed or unsigned,
then we have no idea what the test "status < PGRES_EMPTY_QUERY" even
means.  So I think the most reasonable fix is probably
if ((unsigned int) status >= sizeof pgresStatus / sizeof pgresStatus[0])

which is sufficient to cover both directions, since if status is passed
as -1 then it will convert to a large unsigned value.  It's also a
natural expression of what we really want, ie, that the integer
equivalent of the enum value is in range.
        regards, tom lane

On 3 August 2011 15:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> No, this is not an improvement at all.  The point of the code is that we
> are about to use the enum value as an integer array subscript, and we
> want to make sure it is within the array bounds.  Tying that logic to
> some member of the enum is not a readability or safety improvement.
> We aren't trusting the caller to pass a valid enum value, and likewise
> this code doesn't particularly want to trust the enum definition to be
> anything in particular.

I would think that if someone were to have a reason to change the
explicit value set for the identifier PGRES_EMPTY_QUERY, they would
carefully consider the ramifications of doing so. It's far more likely
they'd just append new values to the end of the enum though. This is
why I don't consider that what I've proposed exposes us to any
additional risk.

> There is another point here, though, which is that if we're not sure
> whether the compiler considers ExecStatusType to be signed or unsigned,
> then we have no idea what the test "status < PGRES_EMPTY_QUERY" even
> means.

I'm sorry, but I don't know what you mean by this.

> So I think the most reasonable fix is probably
>
>        if ((unsigned int) status >= sizeof pgresStatus / sizeof pgresStatus[0])
>
> which is sufficient to cover both directions, since if status is passed
> as -1 then it will convert to a large unsigned value.  It's also a
> natural expression of what we really want, ie, that the integer
> equivalent of the enum value is in range.

I'm not convinced that that is an improvement to rely on the
conversion doing so, but it's not as if I feel very strongly about it.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On ons, 2011-08-03 at 10:25 +0100, Peter Geoghegan wrote:
> Attached patch removes the tautologolical part of an evaluated
> expression, fixing the problem flagged by this quite valid warning.

I think these warnings are completely bogus and should not be worked
around.  Even in the most trivial case of

{   unsigned int foo;
   ...
   if (foo < 0 && ...)       ...
}

I would not want to remove the check, because as the code gets moved
around, refactored, reused, whatever, the unsigned int might change into
a signed int, and then you're hosed.  It's fine for -Wextra, but not for
the -Wall level.

On ons, 2011-08-03 at 10:33 -0700, David E. Wheeler wrote:
> I haven't been paying attention to warnings, but FWIW 9.1beta3 has
> been building with LLVM by default with Xcode 4. Both on Snow Leopard
> and on Lion I saw something like this:
> 
> try=# select version();
> 
> version                                                                                   
>
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>  PostgreSQL 9.1beta3 on x86_64-apple-darwin11.0.0, compiled by
> i686-apple-darwin11-llvm-gcc-4.2 (GCC) 4.2.1 (Based on Apple Inc.
> build 5658) (LLVM build 2335.15.00), 64-bit
> (1 row)
> 
> Which is just awesome. Things seem to work great.

Note that what you are using there is a GCC frontend with a LLVM
backend.  (So I suppose you would get mostly GCC warnings.)  Clang is a
new/different compiler frontend using the LLVM backend.

On 3 Aug 2011, at 19:20, David E. Wheeler wrote:

>
> Yeah, not sure whether or not to tweak the Makefile to use Clang. I guess not?
>

export CC=clang
./configure
...

Peter Geoghegan <peter@2ndquadrant.com> writes:
> On 3 August 2011 15:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> There is another point here, though, which is that if we're not sure
>> whether the compiler considers ExecStatusType to be signed or unsigned,
>> then we have no idea what the test "status < PGRES_EMPTY_QUERY" even
>> means.

> I'm sorry, but I don't know what you mean by this.

I mean that it's unclear what you'll get if status has a bitpattern
equivalent to a negative integer.  If the compiler implements the
comparison as signed, the test will yield TRUE; if unsigned, it's FALSE.

>> So I think the most reasonable fix is probably
>> 
>>        if ((unsigned int) status >= sizeof pgresStatus / sizeof pgresStatus[0])
>> 
>> which is sufficient to cover both directions, since if status is passed
>> as -1 then it will convert to a large unsigned value.  It's also a
>> natural expression of what we really want, ie, that the integer
>> equivalent of the enum value is in range.

> I'm not convinced that that is an improvement to rely on the
> conversion doing so, but it's not as if I feel very strongly about it.

The C standard specifies that signed-to-unsigned conversions must work
like that; and even if the standard didn't, it would surely work like
that on any machine with two's-complement representation, which is to
say every computer built in the last forty years or so.  So I don't find
it a questionable assumption.
        regards, tom lane

On Wed, Aug 03, 2011 at 04:03:39PM -0400, Tom Lane wrote:
> The C standard specifies that signed-to-unsigned conversions must work
> like that; and even if the standard didn't, it would surely work like
> that on any machine with two's-complement representation, which is to
> say every computer built in the last forty years or so.  So I don't find
> it a questionable assumption.

I had the "pleasure" of working on a Univac 1108 in about 1978 and
it was very definitely ones complement. I'm somewhat amazed to find that
the Univac 1100 series architecture and instruction set lives on to this
day. The last pure 1100 seems to be the Unisys 2200/3800 released in 1997.
Even later U1100/Exec 8 descendants appear to still exist and are still
actively supported:
 http://en.wikipedia.org/wiki/Unisys_OS_2200_operating_system

So there are still ones complement machines out there. However I suggest we
pretend otherwise and continue to ignore them.

-dg

--
David Gould       daveg@sonic.net      510 536 1443    510 282 0869
If simplicity worked, the world would be overrun with insects.

On 3 August 2011 21:03, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I mean that it's unclear what you'll get if status has a bitpattern
> equivalent to a negative integer.  If the compiler implements the
> comparison as signed, the test will yield TRUE; if unsigned, it's FALSE.

On compilers on which the enum value is represented as an unsigned
int, passing -1 is just the same as doing that with any function with
an unsigned int argument for that argument - it will convert to a
large unsigned value . So sure, that isolated part of the test's
outcome will vary depending on whether or not the compiler opts to
represent the enum as signed when it can, but I don't look at it as
you do. I simply consider that to be a violation of the enum's
contract, or the caller's failure to consider that the enum couldn't
represent -1 -- they got what they asked for.

To put it another way, you could say that the first part of the test
only exists for the benefit of compilers that treat all enums as
signed. Clang recognised that redundancy for its unsigned case, and
complained. It doesn't make sense to me to look at that one part of
the test in isolation though. With my patch, the test as a whole does
its job (in an identical fashion to before, in fact).

Come to think of it, you could argue that the warning is invalid on
the basis that the enum being unsigned is implementation defined and
therefore the first part of the test is conceivably not redundant on
some platforms, and perhaps I will on the Clang list. Probably not
though, because it was hard enough with the warning bug that I managed
to get them to fix, and I thought that was open-and-shut.

>> I'm not convinced that that is an improvement to rely on the
>> conversion doing so, but it's not as if I feel very strongly about it.
>
> The C standard specifies that signed-to-unsigned conversions must work
> like that; and even if the standard didn't, it would surely work like
> that on any machine with two's-complement representation, which is to
> say every computer built in the last forty years or so.  So I don't find
> it a questionable assumption.

I wasn't questioning whether it would work. I just think that relying
on the behaviour of the conversion like that doesn't make your intent
very clear. If I thought that it would or could be plain broken under
certain circumstances, I would naturally feel strongly about it; as
I've said, I don't.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On Wed, Aug 03, 2011 at 01:40:42PM +0300, Heikki Linnakangas wrote:
> On 03.08.2011 13:05, Peter Geoghegan wrote:
> >I don't believe that the standard allows for an implementation of
> >enums as unsigned integers - after all, individual enum literals can
> >be given corresponding negative integer values.
> 
> C99 says:
> 
> >Each enumerated type shall be compatible with char, a signed integer type, or an
> >unsigned integer type. The choice of type is implementation-defined,110) but shall be
> >capable of representing the values of all the members of the enumeration.

Are we moving to C99?

C89 says:
   Each enumerated type shall be compatible with an integer type; the   choice of type is implementation-defined.

Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On 04.08.2011 04:21, David Fetter wrote:
> On Wed, Aug 03, 2011 at 01:40:42PM +0300, Heikki Linnakangas wrote:
>> On 03.08.2011 13:05, Peter Geoghegan wrote:
>>> I don't believe that the standard allows for an implementation of
>>> enums as unsigned integers - after all, individual enum literals can
>>> be given corresponding negative integer values.
>>
>> C99 says:
>>
>>> Each enumerated type shall be compatible with char, a signed integer type, or an
>>> unsigned integer type. The choice of type is implementation-defined,110) but shall be
>>> capable of representing the values of all the members of the enumeration.
>
> Are we moving to C99?
>
> C89 says:
>
>      Each enumerated type shall be compatible with an integer type; the
>      choice of type is implementation-defined.

Well, that's the same thing, just in less explicit words.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com

Peter Geoghegan <peter@2ndquadrant.com> writes:
> On 3 August 2011 21:03, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I mean that it's unclear what you'll get if status has a bitpattern
>> equivalent to a negative integer. �If the compiler implements the
>> comparison as signed, the test will yield TRUE; if unsigned, it's FALSE.

> On compilers on which the enum value is represented as an unsigned
> int, passing -1 is just the same as doing that with any function with
> an unsigned int argument for that argument - it will convert to a
> large unsigned value . So sure, that isolated part of the test's
> outcome will vary depending on whether or not the compiler opts to
> represent the enum as signed when it can, but I don't look at it as
> you do. I simply consider that to be a violation of the enum's
> contract, or the caller's failure to consider that the enum couldn't
> represent -1 -- they got what they asked for.

This argument is completely missing the point of the test, which is to
verify whether or not the caller adhered to the enum's contract.  You
can *not* assume that he did while arguing about whether the test is
valid or accomplishes its goals.
        regards, tom lane

On 4 August 2011 07:08, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Peter Geoghegan <peter@2ndquadrant.com> writes:
>> On 3 August 2011 21:03, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I mean that it's unclear what you'll get if status has a bitpattern
>>> equivalent to a negative integer.  If the compiler implements the
>>> comparison as signed, the test will yield TRUE; if unsigned, it's FALSE.
>
>> On compilers on which the enum value is represented as an unsigned
>> int, passing -1 is just the same as doing that with any function with
>> an unsigned int argument for that argument - it will convert to a
>> large unsigned value . So sure, that isolated part of the test's
>> outcome will vary depending on whether or not the compiler opts to
>> represent the enum as signed when it can, but I don't look at it as
>> you do. I simply consider that to be a violation of the enum's
>> contract, or the caller's failure to consider that the enum couldn't
>> represent -1 -- they got what they asked for.
>
> This argument is completely missing the point of the test, which is to
> verify whether or not the caller adhered to the enum's contract.  You
> can *not* assume that he did while arguing about whether the test is
> valid or accomplishes its goals.

I did not assume anything about the caller or their trustworthiness.
The whole point of my argument is that passing a negative integer
where the enum is represented as unsigned is just another way of
violating the contract (passing a negative integer where the enum is
represented as signed is another), that is equally well handled by the
test; the whole test though, not the isolated part of it that you
referred to.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

Can we please commit a fix for this problem?

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On Thu, Aug 4, 2011 at 4:49 PM, Peter Geoghegan <peter@2ndquadrant.com> wrote:
> Can we please commit a fix for this problem?

Done.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 5 August 2011 17:12, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Aug 4, 2011 at 4:49 PM, Peter Geoghegan <peter@2ndquadrant.com> wrote:
>> Can we please commit a fix for this problem?
>
> Done.

Thanks Robert.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

Now, apart from the Flex warning, there are just 3 warnings left. They
all look like this:

repl_gram.y:106:30: warning: implicit conversion from enumeration type
'enum ReplNodeTag' to different enumeration type 'NodeTag' (aka 'enum
NodeTag') [-Wconversion]
                                        (yyval.node) = (Node *)
makeNode(IdentifySystemCmd);

^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../src/include/nodes/nodes.h:475:64: note: expanded from:
#define makeNode(_type_)                ((_type_ *)
newNode(sizeof(_type_),T_##_type_))
                                                                             ^
<scratch space>:180:1: note: expanded from:
T_IdentifySystemCmd
^
../../../src/include/nodes/nodes.h:452:19: note: expanded from:
        _result->type = (tag); \
                      ~  ^~~
Attached patch fixes all 3 warnings with an explicit cast, so the
number of warnings with Clang is the same number as GCC 4.5 - 1. On
GCC 4.6, there are still quite a few -Wunused-but-set-variable
warnings left despite an effort to eradicate them. Perhaps I should
look into that next.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

Peter Geoghegan <peter@2ndquadrant.com> writes:
> Now, apart from the Flex warning, there are just 3 warnings left. They
> all look like this:

> repl_gram.y:106:30: warning: implicit conversion from enumeration type
> 'enum ReplNodeTag' to different enumeration type 'NodeTag' (aka 'enum
> NodeTag') [-Wconversion]

> Attached patch fixes all 3 warnings with an explicit cast,

This patch is a truly horrid idea, as it eliminates the error checking
the compiler is trying to provide, and does so globally not only in the
trouble spots.

If I were trying to get rid of this warning, I'd be wondering why
ReplNodeTag is a distinct enum in the first place.  Surely it does not
make sense to be using the Node mechanisms on something that isn't
conforming to the standard node numbering.  If these nodes ever got
passed to anything else in the system, they'd get misinterpreted.
So I'm inclined to think that clang has pointed out a real issue,
rather than something we ought to paper over.
        regards, tom lane

On 5 August 2011 20:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> This patch is a truly horrid idea, as it eliminates the error checking
> the compiler is trying to provide, and does so globally not only in the
> trouble spots.

I think that the mistake I may have made here was assuming that the
existing code was perfect, and therefore it was a simply matter of
tweaking. With that assumption in mind, the only fix could have been a
global fix. I should not have acted in haste.

> If I were trying to get rid of this warning, I'd be wondering why
> ReplNodeTag is a distinct enum in the first place.

Indeed, that doesn't seem to be justified anywhere, and seems to be a
violation of the abstraction of Node as a pseudo base class which
would have broken any "downcasting" that we might have attempted to
do. Since ReplNodeTag wasn't being used directly, just its enumeration
constants, simply moving the constants to the global, generic NodeTag
enum fixes the issue.

That is what the attached patch does.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

Peter Geoghegan <peter@2ndquadrant.com> writes:
> On 5 August 2011 20:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> If I were trying to get rid of this warning, I'd be wondering why
>> ReplNodeTag is a distinct enum in the first place.

> Indeed, that doesn't seem to be justified anywhere, and seems to be a
> violation of the abstraction of Node as a pseudo base class which
> would have broken any "downcasting" that we might have attempted to
> do. Since ReplNodeTag wasn't being used directly, just its enumeration
> constants, simply moving the constants to the global, generic NodeTag
> enum fixes the issue.

> That is what the attached patch does.

I did this plus moving replnodes.h into a saner spot: if they're
full-fledged Nodes, they ought to be defined in src/include/nodes/.

I did not renumber the existing node tags as your patch suggests.
We might want to do that at some point to make a bit more breathing
room, but I think it's too late in the 9.1 cycle to be doing that
in 9.1.  People have probably already built third-party code that
incorporates values like T_ReturnSetInfo.
        regards, tom lane

On Aug 3, 2011, at 12:21 PM, David E. Wheeler wrote:

> On Aug 3, 2011, at 11:28 AM, Grzegorz Jaskiewicz wrote:
>
>> export CC=clang
>> ./configure
>> ...
>
> Thanks, I'll give that a try the next time I build (RC1 I guess).

FYI, I just built Beta3 using Clang on Mac OS X 10.7 (Darwin 11). The build script looks like this:

    export VERSION=9.1beta3
    export PERL=/usr/local/bin/perl
    export BASE=/usr/local/pgsql

    export CC=clang
     # For debugging: --enable-cassert --enable-debug
    ./configure --with-bonjour --with-perl PERL=$PERL \
    --with-openssl --with-pam --with-krb5 --with-libxml \
    --with-ossp-uuid --with-includes=/usr/local/include \
    --enable-integer-datetimes --with-zlib \
    --with-libs=/usr/local/lib --prefix=$BASE || exit $?

    make world -j3 # || exit $?
    make install-world || exit $?

    mkdir $BASE/data
    chmod 0700 $BASE/data
    chown -R postgres:postgres $BASE/data
    sudo -u postgres $BASE/bin/initdb --locale en_US.UTF-8 --encoding utf-8 -D $BASE/data
    mkdir $BASE/data/logs
    chown -R postgres:postgres $BASE/data/logs

I figure there might be warnings you haven't seen if you haven't been building with bonjour, perl, openssl, pam,
libxml,or ossp-uuid, so I've attached the output. Pity you can't see the pretty colors the Clang sends to the terminal.
:-)

Best,

David

On fre, 2011-08-12 at 11:46 -0700, David E. Wheeler wrote:
> I figure there might be warnings you haven't seen if you haven't been
> building with bonjour, perl, openssl, pam, libxml, or ossp-uuid, so
> I've attached the output.

We have a build farm member (smew) that covers all of that except
bonjour, but I can confirm that there are no additional warnings on
account of that.

On Aug 12, 2011, at 12:09 PM, Peter Eisentraut wrote:

>> I figure there might be warnings you haven't seen if you haven't been
>> building with bonjour, perl, openssl, pam, libxml, or ossp-uuid, so
>> I've attached the output.
> 
> We have a build farm member (smew) that covers all of that except
> bonjour, but I can confirm that there are no additional warnings on
> account of that.

Ah, great, thanks!

David

On 12 August 2011 20:10, David E. Wheeler <david@kineticode.com> wrote:
> Ah, great, thanks!

Unfortunately, you'll need to build your own Clang to be up to speed
on the work I've done here, because the Clang developers both fixed
the spurious warning from assigning past what appears to be the end of
an array, plus a significant performance issue when building gram.c
and other grammar files. Building Clang is not difficult, but it's a
little time-consuming.

I imagine we'll have to wait until the release and wide availability
of Clang 3 before we start to see people using it for day-to-day
hacking on Postgres.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

<p>I realize it's a bit late to jump in here with the path already having been committed. But I think there's a point
thatwas missed in the discussion. One reason to do the test as Tom recommended is that the warning probably indicates
thatthe test as written was just going to be optimized away as dead code. I think the cast to unsigned is the least
likelyidiom to be optimized away whereas any of the formulations based on comparing the enum with enum labels is quite
likelyto be.