Bruce Momjian <br...@momjian.us> wrote: > On Tue, Jun 18, 2013 at 11:04:25AM -0700, Kevin Grittner wrote: >> D'Arcy J.M. Cain <da...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers