Andres Freund <and...@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 <and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
