Re: Further news on Clang - spurious warnings

From: Peter Geoghegan <peter(at)2ndquadrant(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Further news on Clang - spurious warnings
Date: 2011-08-03 22:58:51
Message-ID: CAEYLb_Wr5O7=+jpWCrZpkytSe-6zEFLJdsz+Q7OSV51ufgrY5A@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 3 August 2011 21:03, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2011-08-04 01:16:14 Re: mosbench revisited
Previous Message Tom Lane 2011-08-03 22:48:38 Re: Postgres / plpgsql equivalent to python's getattr() ?