Skip site navigation (1) Skip section navigation (2)

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 15:31:10
Message-ID: CAEYLb_X2Tv5pDLmtER=LkX=p9eVTF+nF093E0JBUF3d89b9Q=A@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
On 3 August 2011 15:29, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

In response to

Responses

pgsql-hackers by date

Next:From: Robert HaasDate: 2011-08-03 15:55:31
Subject: Re: WIP fix proposal for bug #6123
Previous:From: Tom LaneDate: 2011-08-03 15:18:20
Subject: Re: error: could not find pg_class tuple for index 2662

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group