On 10/5/19 12:16 AM, Bernd Edlinger wrote: > > > On 10/4/19 10:26 PM, Jeff Law wrote: >> On 10/4/19 12:24 PM, Bernd Edlinger wrote: >>> Hi, >>> >>> these macros don't use reserved names for local variables. >>> This caused -Wshadow=local warnings in varasm.c IIRC. >>> >>> Fixed by using _lowercase reserved names for macro parameters. >>> >>> >>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. >>> Is it OK for trunk? >>> >>> >>> Thanks >>> Bernd. >>> >>> >>> >>> patch-wshadow-elfos.diff >>> >>> 2019-10-04 Bernd Edlinger <bernd.edlin...@hotmail.de> >>> >>> * config/elfos.h (ASM_DECLARE_OBJECT_NAME, >>> ASM_FINISH_DECLARE_OBJECT): Rename local vars in macro. >> Same objections as before. As long as we're using macros like this, >> we're going to have increased potential for shadowing problems and >> macros which touch implementation details that just happen to be >> available in the context where the macro is used. >> >> Convert to real functions. It avoids the shadowing problem and avoids >> macros touching/referencing things they shouldn't. Code in macros may >> have been reasonable in the 80s/90s, but we should know better by now. >> >> I'm not ranting against you Bernd, it's more a rant against the original >> coding style for GCC. Your changes just highlight how bad of an idea >> this kind of macro usage really is. We should take the opportunity to >> fix this stuff for real. >> > > I understand that, and would not propose to fix it this way if this > was the *only* place that breaks with -Wshadow=local. > > I will continue to send more patches over the weekend, formally obvious > ones, but the amount of changes is just huge. > > I would suggest I send them here, and wait for at least 24H before > committing, so anybody can suggest better names, for instance, > or other (small) improvements, as you like. I've been away for the last week. I hope you've seen that the concerns folks are raising are a good indicator that the current approach isn't how we want to fix these issues.
Factoring/refactoring, hooks, and _sensible_ renaming of variables are the way forward. Underscores and numeric prefixes/suffixes are just making things worse. My recommendation would be to look at an opt-out style. ie, add the new warning, but explicitly disable it (in Makefile.in) for every file were we're not clean yet. There's already some mechanisms to do this in Makefile.in where we disable specific warnings for specific files. Then address one file at a time in the right way then remove that file from the opt-out list. Continue until done :-) I realize this will be a lot of work. jeff
