On Thu, Aug 18, 2022 at 5:27 PM David Rowley <dgrowle...@gmail.com> wrote: > > On Thu, 18 Aug 2022 at 17:16, Justin Pryzby <pry...@telsasoft.com> wrote: > > > > On Thu, Aug 18, 2022 at 03:17:33PM +1200, David Rowley wrote: > > > I'm probably not the only committer to want to run a mile when they > > > see someone posting 17 or 26 patches in an email. So maybe "bang for > > > buck" is a better method for getting the ball rolling here. As you > > > know, I was recently bitten by local shadows in af7d270dd, so I do > > > believe in the cause. > > > > > > What do you think? > > > > You already fixed the shadow var introduced in master/pg16, and I sent > > patches > > for the shadow vars added in pg15 (marked as such and presented as > > 001-008), so > > perhaps it's okay to start with that ? > > Alright, I made a pass over the 0001-0008 patches. > ...
> > 0005. How about just "tslot". I'm not a fan of "this". > (I'm sure there are others like this; I just picked this one as an example) AFAICT the offending 'slot' really should have never been declared at all at the local scope in the first place - e.g. the other code in this function seems happy enough with the pattern of just re-using the function scoped 'slot'. I understand that for this shadow patch changing the var-name is considered the saner/safer way than tampering with the scope, but perhaps it is still useful to include a comment when changing ones like this? e.g. + TupleTableSlot *tslot; /* TODO - Why declare this at all? Shouldn't it just re-use the 'slot' at function scope? */ Otherwise, such knowledge will be lost, and nobody will ever know to revisit them, which feels a bit more like *hiding* the mistake than fixing it. ------ Kind Regards, Peter Smith. Fujitsu Australia
