| Message ID | AM6PR03MB4519D1AF481BE2875427F477E49E0@AM6PR03MB4519.eurprd03.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix -Wshadow=local warnings in defaults.h | expand |
On 10/4/19 12:24 PM, Bernd Edlinger wrote: > Hi, > > this macro caused -Wshadow=local warnings in varasm.c with > the microblaze target. > > > Only built a bare metal cross compiler that was able to compile > libgcc for that target. > > Is it OK for trunk? > > > Thanks > Bernd. > > > > patch-wshadow-defaults.diff > > 2019-10-04 Bernd Edlinger <bernd.edlinger@hotmail.de> > > * defaults.h (ASM_OUTPUT_ASCII): Rename local vars in macro. > Remove variable hiding code. For the record, I really dislike blindly prefixing or suffixing variables with numbers or underscores to avoid shadow warnings. IMHO it's not a significant improvement in terms of readability. Just to be clear, I'm absolutely for fixing our shadowing problems, but I'm not sure the way we're going about it is actually a significant improvement. jeff
On 10/4/19 12:24 PM, Bernd Edlinger wrote: > Hi, > > this macro caused -Wshadow=local warnings in varasm.c with > the microblaze target. > > > Only built a bare metal cross compiler that was able to compile > libgcc for that target. > > Is it OK for trunk? > > > Thanks > Bernd. > > > > patch-wshadow-defaults.diff > > 2019-10-04 Bernd Edlinger <bernd.edlinger@hotmail.de> > > * defaults.h (ASM_OUTPUT_ASCII): Rename local vars in macro. > Remove variable hiding code. ISTM this would be better done by creating a target hook with the defaults.h implementation as the default implementation and converting the dozen or so backends that need to override the default. More generally, shadowing problems are inherent with macro usage and I suspect pervasive due to the original GCC style of writing far too much code in macros. Rather then burning a lot of time renaming variables, which still have shadowing potential that we should be turning all the macro crap into real functions. Jeff
On 10/4/19 10:23 PM, Jeff Law wrote: > On 10/4/19 12:24 PM, Bernd Edlinger wrote: >> Hi, >> >> this macro caused -Wshadow=local warnings in varasm.c with >> the microblaze target. >> >> >> Only built a bare metal cross compiler that was able to compile >> libgcc for that target. >> >> Is it OK for trunk? >> >> >> Thanks >> Bernd. >> >> >> >> patch-wshadow-defaults.diff >> >> 2019-10-04 Bernd Edlinger <bernd.edlinger@hotmail.de> >> >> * defaults.h (ASM_OUTPUT_ASCII): Rename local vars in macro. >> Remove variable hiding code. > ISTM this would be better done by creating a target hook with the > defaults.h implementation as the default implementation and converting > the dozen or so backends that need to override the default. > Yes, absolutely, but I need to do some nit-picking here, in order to make any progress at all. It is out of my scope to do that macro conversion for all ports at the same time when I just want to add a warning. I believe however, that using names in the reserved in the global namespace _ followed by lowercase letter, is still an improvement, as that is much less likely to be used in normal code. > More generally, shadowing problems are inherent with macro usage and I > suspect pervasive due to the original GCC style of writing far too much > code in macros. > This macro is a bit special, in that it is insisting in shadowing local variables purposefully. I call that childish... > Rather then burning a lot of time renaming variables, which still have > shadowing potential that we should be turning all the macro crap into > real functions. > Jeff, this is just the beginning. I have sent so far just about 10% of all changes that I have in my tree. I will send more patches soon, but I need some more time to split the changes up in chunks that are not too big to be sent to this list, and write the proper change logs. Current situation is this: We have lots of 1000+ line functions that shadow variables all the time. I believe nobody did that on purpose, but currently you cannot know, if you assign a value to a variable with whatever name in any function and you see the same variable somewhere later in that function that it is still the same variable. For clarifying the data flow it does not matter if I change name to name1 or some_other_name. I use /name1 in vi anyway. After this is changed, you will always know that once you assing a variable it will have that value until you assign the same variable with the same name again, then it will be the new value, but currently the old value can pop up again. I know these are not very good names: int i, j, k; int i1, i2, i3; In general a good name should not just repeat the data type of the variable, but the intended use. for instance: int idx; would be better, since I know by that name that idx will be used as an array index. But it should also not be too long, mind the 80-column rule. I believe finding a good name is often a truly challenging task of its own. However I think, the amount of changes that I have to do just to get every target with every language, with -Wshadow=local boot-strap without error, makes it impossible to think of achieving a second goal like that, at the same time. After all this is a technical debt that accumulated for 20+ Years now. Bernd.
2019-10-04 Bernd Edlinger <bernd.edlinger@hotmail.de> * defaults.h (ASM_OUTPUT_ASCII): Rename local vars in macro. Remove variable hiding code. Index: gcc/defaults.h =================================================================== --- gcc/defaults.h (revision 276484) +++ gcc/defaults.h (working copy) @@ -61,37 +61,32 @@ see the files COPYING3 and COPYING.RUNTIME respect #ifndef ASM_OUTPUT_ASCII #define ASM_OUTPUT_ASCII(MYFILE, MYSTRING, MYLENGTH) \ do { \ - FILE *_hide_asm_out_file = (MYFILE); \ - const unsigned char *_hide_p = (const unsigned char *) (MYSTRING); \ - int _hide_thissize = (MYLENGTH); \ - { \ - FILE *asm_out_file = _hide_asm_out_file; \ - const unsigned char *p = _hide_p; \ - int thissize = _hide_thissize; \ - int i; \ - fprintf (asm_out_file, "\t.ascii \""); \ + FILE *_asm_out_file = (MYFILE); \ + const unsigned char *_p = (const unsigned char *) (MYSTRING); \ + int _thissize = (MYLENGTH); \ + int _i; \ \ - for (i = 0; i < thissize; i++) \ - { \ - int c = p[i]; \ - if (c == '\"' || c == '\\') \ - putc ('\\', asm_out_file); \ - if (ISPRINT (c)) \ - putc (c, asm_out_file); \ - else \ - { \ - fprintf (asm_out_file, "\\%o", c); \ - /* After an octal-escape, if a digit follows, \ - terminate one string constant and start another. \ - The VAX assembler fails to stop reading the escape \ - after three digits, so this is the only way we \ - can get it to parse the data properly. */ \ - if (i < thissize - 1 && ISDIGIT (p[i + 1])) \ - fprintf (asm_out_file, "\"\n\t.ascii \""); \ + fprintf (_asm_out_file, "\t.ascii \""); \ + for (_i = 0; _i < _thissize; _i++) \ + { \ + int _c = _p[_i]; \ + if (_c == '\"' || _c == '\\') \ + putc ('\\', _asm_out_file); \ + if (ISPRINT (_c)) \ + putc (_c, _asm_out_file); \ + else \ + { \ + fprintf (_asm_out_file, "\\%o", _c); \ + /* After an octal-escape, if a digit follows, \ + terminate one string constant and start another. \ + The VAX assembler fails to stop reading the escape \ + after three digits, so this is the only way we \ + can get it to parse the data properly. */ \ + if (_i < _thissize - 1 && ISDIGIT (_p[_i + 1])) \ + fprintf (_asm_out_file, "\"\n\t.ascii \""); \ } \ - } \ - fprintf (asm_out_file, "\"\n"); \ - } \ + } \ + fprintf (_asm_out_file, "\"\n"); \ } \ while (0) #endif