Re: Further news on Clang - spurious warnings

Peter Geoghegan <peter(at)2ndquadrant(dot)com> writes:
> On 3 August 2011 12:19, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)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