Hi,
On Sat, Nov 05, 2022 at 05:04:31PM +0100, Tomas Vondra wrote:
>
> I did a quick initial review of this patch series - attached is a
> version with "review" commits for some of the parts. The current patch
> seems in pretty good shape, most of what I noticed are minor issues. I
> plan to do a more thorough review later.
Thanks!
I agree with all of your comments, just a few answers below
> - NamesFromList and IdentifyVariable seem introduced unnecessarily
> early, as they are only used in 0002 and 0003 parts (in the original
> patch series). Not sure if the plan is to squash everything into a
> single patch, or commit individual patches.
The split was mostly done to make the patch easier to review, as it adds quite
a bit of infrastructure.
There have been some previous comments to have a more logical separation and
fix similar issues, but there are still probably other oddities like that
laying around. I personally didn't focus much on it as I don't know if the
future committer will choose to squash everything or not.
> - AFAIK patches don't need to modify typedefs.list.
I think this was discussed a year or so ago, and my understanding is that the
general rule is that it's now welcome, if not recommended, to maintain
typedefs.list in each patchset.
> Which I think means this:
>
> if (filter_lxid && svar->drop_lxid == MyProc->lxid)
> continue;
>
> accesses drop_lxid, which was not initialized in init_session_variable.
Agreed.
