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 I was 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
