On Mon, Feb 18, 2019 at 5:28 PM, Tom Lane wrote:
> Frankly, that code is just horrid. Having a function with side effects
> in an if-test is questionable at the best of times, and having it be the
> second of three conditions (which the third condition silently depends
> on) is unreadable and unmaintainable.
When I reviewed this, I thought there are no problems in the codes, but I googled what Tom pointed out[1], read it and
Iwas ashamed of my ignorance.
> I think the existing code here is considerably cleaner than what this
> patch proposes.
>
> I suppose you are doing this because you intend to jam some additional
> cleanup code into the successfully-pruned-it code path, but if said code
> is really too bulky to have multiple copies of, couldn't you put it into
> a subroutine?
ISTM the 0004 patch eventually removes these codes from multiple places (set_append_rel_size and
set_inherited_target_rel_sizes)so we might be better to not be struggling here?
[1] https://www.teamten.com/lawrence/programming/keep-if-clauses-side-effect-free.html
--
Yoshikazu Imai
