Andres Freund <andres@2ndquadrant.com> wrote:
> On 2013-06-18 05:21:15 -0400, D'Arcy J.M. Cain wrote:
>> On Tue, 18 Jun 2013 11:01:28 +0200
>> Andres Freund <andres@2ndquadrant.com> wrote:
>> > > /*
>> > > * return true if attnum is out of range according to the tupdesc
>> > > */
>> > > if (attnum > tupleDesc->natts)
>> > > return true;
>> >
>> > I think the comment is more meaningfull before the change since it
>> > tells us how nonexisting columns are interpreted.
>>
>> I think that the comment is bad either way. Comments should explain
>> the code, not repeat it. The above is not far removed from...
>>
>> return 5; /* return the number 5 */
I agree with this -- the comment as it stands adds no information
to what is obvious from a glance at the code, and may waste the
time it takes to mentally resolve the discrepancy between "return
NULL" in the comment and "return true;" in the statement. Unless
it adds information, we'd be better off deleting the comment.
>> How about "check if attnum is out of range according to the
>> tupdesc" instead?
>
> I can't follow. Minus the word 'NULL' - which carries meaning - your
> suggested comment pretty much is the same as the existing comment except
> that you use 'check' instead of 'return'.
The word "indicate" might waste a few milliseconds less in the
double-take; but better would be some explanation of why you might
have an attnum value greater than the maximum, and why the right
thing to do is indicate NULL rather than throwing an error.
--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
