On Sat, 2008-03-22 at 21:35 -0400, Tom Lane wrote:
> After a quick read, looks sane except for one stylistic gripe:
> in exec_stmt_return_next, you added an initialization of tuple = NULL
> in order to remove a couple of lines like
>
> tuple = NULL; /* keep compiler quiet */
>
> I think this is not good coding style. The point of not having the
> initialization is so that the compiler will warn us if there are
> any paths through the function in which we fail to set "tuple".
> You've just disabled that bit of early-warning error detection.
> It's better if each switch arm has to set tuple for itself.
> (If only a minority of them needed to do it, I might think
> differently, but in this case I'd vote for sticking with the
> way it's being done.)
I agree with your general reasoning, but in this case, it seemed cleaner
to initialize "tuple" when it is declared: only 2 of the 6 branches in
the function actually initialize tuple to a non-NULL value.
I think the warn-about-uninitialized-value argument doesn't hold much
water in this particular case, either: each branch of the function
either makes use of "tuple", or does not. When "tuple" is utilized, we
want to push it into the tuplestore; otherwise, we want to do nothing.
Since "tuple" is only used in two cases, it seems cleaner to me to have
the default be "do nothing", and only deviate from it where needed.
-Neil
