Re: nitpick about poor style in MergeAttributes

On Thu, May 23, 2019 at 7:54 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Michael Paquier <michael(at)paquier(dot)xyz> writes:
> > On Wed, May 22, 2019 at 06:20:01PM -0700, Mark Dilger wrote:
> >> What to do about this is harder to say. In the following
> >> patch, I'm just doing what I think is standard for callers
> >> of list_delete_cell, and assigning the return value back
> >> to the list (similar to how a call to repalloc should do).
> >> But since there is an implicit assumption that the list
> >> is never emptied by this operation, perhaps checking
> >> against NIL and elog'ing makes more sense?
>
> > Yes, I agree that this is a bit fuzzy, and this code is new as of
> > 705d433. As you say, I agree that making sure that the return value
> > of list_delete_cell is not NIL is a sensible choice.
>
> Are we sure that's not just a newly-introduced bug, ie it has not
> been tested in cases where the tlist could become empty? My first
> thought would be to assign the list pointer value back as per usual
> coding convention, not to double down on the assumption that this
> was well-considered code.

I don't think that is disputed. I was debating between assigning
it back and also asserting that it is not NIL vs. assigning it back
and elog/ereporting if it is NIL. Of course, this is assuming the
code was designed with the expectation that the list can never
become empty. If you think it might become empty, and that the
surrounding code can handle that sensibly, then perhaps we
need neither the assertion nor the elog/ereport, though we still
need the assignment.

mark