On Fri, Aug 19, 2022 at 9:21 AM Justin Pryzby <pry...@telsasoft.com> wrote: > > On Thu, Aug 18, 2022 at 07:27:09PM +1200, David Rowley wrote: > > 0001. I'd also rather see these 4 renamed: > .. > > 0002. I don't really like the "my" name. I also see you've added the > .. > > How about just "tinfo"? > .. > > 0005. How about just "tslot". I'm not a fan of "this". > .. > > Since I'm not a fan of "this", how about the outer one gets renamed > .. > > 0007. Meh, more "this". How about just "col". > .. > > 0008. Sorry, I had to change this one too. > > I agree that ii_oid and newtype are better names (although it's a bit > unfortunate to rename the outer "ttype" var of wider scope). > > > 0003. The following is used for the exact same purpose as its shadowed > > counterpart. I suggest just using the variable from the outer scope. > > And that's what my original patch did, before people insisted that the patches > shouldn't change variable scope. Now it's back to where I stared. > > > There's a discussion about reverting this entire patch. Not sure if > > patching master and not backpatching to pg15 would be useful to the > > people who may be doing that revert. > > I think if it were reverted, it'd be in both branches. > > > I've attached a patch which does things more along the lines of how I > > would have done it. I don't think we should be back patching this > > stuff. > > > > Any objections to pushing this to master only? > > I won't object, but some of your changes are what makes backpatching this less > reasonable (foreach_current_index and newtype). I had made these v15 patches > first to simplify backpatching, since having the same code in v15 means that > there's no backpatch hazard for this new-in-v15 code. > > I am opened to presenting the patches differently, but we need to come up with > a better process than one person writing patches and someone else rewriting > it. > I also don't see the value of debating which order to write the patches in. > Grouping by variable name or doing other statistical analysis doesn't change > the fact that there are 50+ issues to address to allow -Wshadow to be usable. > > Maybe these would be helpful ? > - if I publish the patches on github; > - if I send the patches with more context; > - if you have an suggestion/objection/complaint with a patch, I can address > it > and/or re-arrange the patchset so this is later, and all the polished > patches are presented first. >
Starting off with patches might come to grief, and it won't be much fun rearranging patches over and over. Because there are so many changes, I think it would be better to attack this task methodically: STEP 1 - Capture every shadow warning and categorise exactly what kind is it. e.g maybe do this as some XLS which can be shared. The last time I looked there were hundreds of instances, but I expect there will be less than a couple of dozen different *categories* of them. e.g. shadow of a global var e.g. shadow of a function param e.g. shadow of a function var in a code block for the exact same usage e.g. shadow of a function var in a code block for some 'tmp' var e.g. shadow of a function var in a code block due to a mistake e.g. shadow of a function var by some loop index e.g. shadow of a function var for some loop 'first' handling e.g. bug etc... STEP 2 - Define your rules for how intend to address each of these kinds of shadows (e.g. just simple rename of the var, use 'foreach_current_index', ...). Hopefully, it will be easy to reach an agreement now since all instances of the same kind will look pretty much the same. STEP 3 - Fix all of the same kinds of shadows per single patch (using the already agreed fix approach from step 2). REPEAT STEPS 2,3 until done. ------ Kind Regards, Peter Smith. Fujitsu Australia
