On Wed, 12 Mar 2025 at 07:45, Richard Guo <guofenglinux@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
