Re: REVIEW: patch: remove redundant code from pl_exec.c

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> While I can certainly appreciate wanting to remove redundant code, I
> don't think this change actually improves the situation. The problem is
> more than just that we might want to make a change to 'while' but not
> 'for', it's also that it makes it very difficult to follow the code
> flow.

That was my reaction as well; and I was also concerned that this could
have a non-negligible performance price. (At the very least it's adding
an additional function call per loop execution, and there could also be
a penalty from forcing "rc" to be in memory rather than a register.)

I think we should reject this one.

> In the end, I'd recommend cleaning up the handling of the exec_stmts()
> return code so that all of these pieces follow the same style and look
> similar (I'd go with the switch-based approach and remove the if/else
> branches). That'll make it easier for anyone coming along later who
> does end up needing to change all three.

Using a switch there is a bit problematic since in some cases you want
to use "break" to exit the loop. We could replace such breaks by gotos,
but that would be another strike against the argument that you're making
things more readable. I think the switch in exec_stmt_loop is only
workable because it has no cleanup to do, so it can just "return" in
places where a loop break would otherwise be needed. In short: if you
want to make these all look alike, better to go the other way.

regards, tom lane