On 3 August 2011 15:29, Tom Lane <t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
