On Wed, Aug 24, 2022 at 10:47:31PM +1200, David Rowley wrote:
> I really think #2s should be done last. I'm not as comfortable with
> the renaming and we might want to discuss tactics on that. We could
> either opt to rename the shadowed or shadowing variable, or both. If
> we rename the shadowing variable, then pending patches or forward
> patches could use the wrong variable. If we rename the shadowed
> variable then it's not impossible that backpatching could go wrong
> where the new code intends to reference the outer variable using the
> newly named variable, but when that's backpatched it uses the variable
> with the same name in the inner scope. Renaming both would make the
> problem more obvious.
The most *likely* outcome of renaming the *outer* variable is that
*every* cherry-pick involving that variable would fails to compile,
which is an *obvious* failure (good) but also kind of annoying if it
could've worked fine if it weren't renamed. I think most of the renames
should be applied to the inner var, because it's of narrower scope, and
more likely to cause a conflict (good) rather than appearing to apply
cleanly but then misbehave. But it seems reasonable to consider
renaming both if the inner scope is longer than a handful of lines.
> Would you be able to write a patch for #4. I'll do #5 now. You could
> do a draft patch for #2 as well, but I think it should be committed
> last, if we decide it's a good move to make. It may be worth having
> the discussion about if we actually want to run
> -Wshadow=compatible-local as a standard build flag before we rename
> anything.
I'm afraid the discussion about default flags would distract from fixing
the individual warnings, which itself preclude usability of the flag by
individual developers, or buildfarm, even as a local setting.
It can't be enabled until *all* the shadows are gone, due to -Werror on
the buildfarm and cirrusci. Unless perhaps we used -Wno-error=shadow.
I suppose we're only talking about enabling it for gcc?
The biggest benefit is if we fix *all* the local shadow vars, since that
allows someone to make use of the option, and thereby avoiding future
such issues. Enabling the option could conceivably avoid issues
cherry-picking into back branch - if an inner var is re-introduced
during conflict resolution, then a new warning would be issued, and
hopefully the developer would look more closely.
Would you check if any of these changes are good enough ?
--
Justin
