On Wed, 12 Mar 2025 at 07:45, Richard Guo <guofengli...@gmail.com> wrote: > > I refined the comment in v2 and ended up with: > > * This analysis could be tighter: in particular, a non-strict > * construct hidden within a lower-level PlaceHolderVar is not > * reason to add another PHV. But for now it doesn't seem > - * worth the code to be more exact. > + * worth the code to be more exact. This is also why it's > + * preferable to handle bare PHVs in the above branch, rather > + * than this branch. We also prefer to handle bare Vars in a > + * separate branch, as it's cheaper this way and parallels the > + * handling of PHVs. >
LGTM. > For backpatching, 0001 seems more like a cosmetic change, and I don't > think it needs to be backpatched. 0002 is a bug fix, but I'm not > confident enough to commit it to the back branches. Given that there > are other wrong result issues with grouping sets fixed in version 18 > but not in earlier versions, and that there are no field reports about > this issue, perhaps it's OK to refrain from backpatching this one? > Yes, I was thinking along the same lines. This particular issue feels a bit manufactured, and unlikely to occur in practice, but I'm happy to go with whatever you decide. Thanks for taking care of this. Regards, Dean
