Bruce Momjian <bruce@momjian.us> wrote:
> On Tue, Jun 18, 2013 at 11:04:25AM -0700, Kevin Grittner wrote:
>> D'Arcy J.M. Cain <darcy@druid.net>
>>
>>> Although, the more I think about it, the more I think that the
>>> comment is both confusing and superfluous. The code itself is
>>> much clearer.
>>
>> Seriously, if there is any comment there at all, it should be a
>> succinct explanation for why we didn't do this
> Is everyone OK with me applying this patch from Kevin, attached?
I guess my intent was misunderstood -- what I was trying to get
across was that the comment added exactly nothing to what you could
get more quickly by reading the code below it. I felt the comment
should either be removed entirely, or a concise explanation of why
it was right thing to do should be there rather than just echoing
the code in English. I wasn't suggesting applying the mini-patch,
but suggesting that *if* we have a comment there at all, it should
make clear why such a patch would be wrong; i.e., why is it is OK
to have attnum > tupdesc->natts here? How would we get to such a
state, and why is NULL the right thing for it? Such a comment
would actually help someone trying to understand the code, rather
than wasting their time. After all, in the same file we have this:
/* Check for caller error */
if (attnum <= 0 || attnum > slot->tts_tupleDescriptor->natts)
elog(ERROR, "invalid attribute number %d", attnum);
An explanation of why it is caller error one place and not another
isn't a waste of space.
>> (which passes `make check-world`)
And I was disappointed that our regression tests don't actually
exercise that code path, which would be another good way to make
the point.
So anyway, *I* would object to applying that; it was meant to
illustrate what the comment, if any, should cover; not to be an
actual code change. I don't think the change that was pushed helps
that comment carry its own weight, either. If we can't do better
than that, we should just drop it.
I guess I won't try to illustrate a point *that* particular way
again....
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company