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.
