Fix -Wshadow=local warnings in defaults.h
diff mbox series

Message ID AM6PR03MB4519D1AF481BE2875427F477E49E0@AM6PR03MB4519.eurprd03.prod.outlook.com
State New
Headers show
Series
  • Fix -Wshadow=local warnings in defaults.h
Related show

Commit Message

Bernd Edlinger Oct. 4, 2019, 6:24 p.m. UTC
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.

Comments

Jeff Law Oct. 4, 2019, 8:15 p.m. UTC | #1
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
Jeff Law Oct. 4, 2019, 8:23 p.m. UTC | #2
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
Bernd Edlinger Oct. 5, 2019, 6:16 a.m. UTC | #3
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.

Patch
diff mbox series

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