[rfa] function parameter shadowed by local variable in opencl-lang.c

[rfa] function parameter shadowed by local variable in opencl-lang.c

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 35 bytes --]

Probably this wasn't intentional?


[-- Attachment #2: shadow.txt --]
[-- Type: text/plain, Size: 848 bytes --]

2011-02-26  Michael Snyder  <msnyder@vmware.com>

	* opencl-lang.c (lval_func_check_synthetic_pointer): Local variable
	shadows function parameter.

Index: opencl-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/opencl-lang.c,v
retrieving revision 1.7
diff -u -p -u -p -r1.7 opencl-lang.c
--- opencl-lang.c	21 Feb 2011 15:53:10 -0000	1.7
+++ opencl-lang.c	26 Feb 2011 23:19:10 -0000
@@ -318,11 +318,11 @@ lval_func_check_synthetic_pointer (const
   for (i = start; i < end; i++)
     {
       int startoffset = (i == start) ? startrest : 0;
-      int length = (i == end) ? endrest : elsize;
+      int length2 = (i == end) ? endrest : elsize;
 
       if (!value_bits_synthetic_pointer (c->val,
 					 c->indices[i] * elsize + startoffset,
-					 length))
+					 length2))
 	return 0;
     }
 

Re: [rfa] function parameter shadowed by local variable in opencl-lang.c

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 106 bytes --]

Michael Snyder wrote:
> Probably this wasn't intentional?
> 

Oops, another instance of the same thing:



[-- Attachment #2: shadow2.txt --]
[-- Type: text/plain, Size: 1096 bytes --]

Index: opencl-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/opencl-lang.c,v
retrieving revision 1.7
diff -u -p -u -p -r1.7 opencl-lang.c
--- opencl-lang.c	21 Feb 2011 15:53:10 -0000	1.7
+++ opencl-lang.c	26 Feb 2011 23:26:07 -0000
@@ -264,10 +264,10 @@ lval_func_check_validity (const struct v
   for (i = start; i < end; i++)
     {
       int startoffset = (i == start) ? startrest : 0;
-      int length = (i == end) ? endrest : elsize;
+      int length2 = (i == end) ? endrest : elsize;
 
       if (!value_bits_valid (c->val, c->indices[i] * elsize + startoffset,
-			     length))
+			     length2))
 	return 0;
     }
 
@@ -318,11 +318,11 @@ lval_func_check_synthetic_pointer (const
   for (i = start; i < end; i++)
     {
       int startoffset = (i == start) ? startrest : 0;
-      int length = (i == end) ? endrest : elsize;
+      int length2 = (i == end) ? endrest : elsize;
 
       if (!value_bits_synthetic_pointer (c->val,
 					 c->indices[i] * elsize + startoffset,
-					 length))
+					 length2))
 	return 0;
     }
 

Re: [rfa] function parameter shadowed by local variable in opencl-lang.c

[Joel Brobecker]

> >Probably this wasn't intentional?
> >
> 
> Oops, another instance of the same thing:

I wish the compiler would warn us about things like this...  The patch
looks correct to me, but I will let Ken and Ulrich take a look. Perhaps
they'll be able to suggest a more specific name than `length2' ;-).

-- 
Joel

[RFC] Compile GDB with -Wshadow? (was: "Re: [rfa] function parameter shadowed by local variable in opencl-lang.c")

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 388 bytes --]

> I wish the compiler would warn us about things like this...  The patch
> looks correct to me, but I will let Ken and Ulrich take a look. Perhaps
> they'll be able to suggest a more specific name than `length2' ;-).

Howzabout we add -Wshadow to the list of warning compiler switches
for GCC? Something like this (untested for now, just to give an idea
for this discussion)...

-- 
Joel

[-- Attachment #2: wshadow.diff --]
[-- Type: text/x-diff, Size: 496 bytes --]

diff --git a/gdb/configure.ac b/gdb/configure.ac
index f31ef2a..ea0cda9 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1753,7 +1753,7 @@ fi
 build_warnings="-Wall -Wdeclaration-after-statement -Wpointer-arith \
 -Wformat-nonliteral -Wno-pointer-sign \
 -Wno-unused -Wunused-value -Wunused-function \
--Wno-switch -Wno-char-subscripts"
+-Wno-switch -Wno-char-subscripts -Wshadow"
 
 # Enable -Wno-format by default when using gcc on mingw since many
 # GCC versions complain about %I64.

Re: [RFC] Compile GDB with -Wshadow? (was: "Re: [rfa] function parameter shadowed by local variable in opencl-lang.c")

[Jan Kratochvil]

On Mon, 28 Feb 2011 05:57:35 +0100, Joel Brobecker wrote:
> Howzabout we add -Wshadow to the list of warning compiler switches
> for GCC?

That would be great but it produces now 627 errors, I am not going to fix
them, they look each one needs a specific fix.


Thanks,
Jan

Re: [RFC] Compile GDB with -Wshadow? (was: "Re: [rfa] function parameter shadowed by local variable in opencl-lang.c")

[Joel Brobecker]

> That would be great but it produces now 627 errors, I am not going to fix
> them, they look each one needs a specific fix.

:-(. Completely impractical at this stage. It's really surprising
that we would have so many errors of this kind...

-- 
Joel

Re: [RFC] Compile GDB with -Wshadow? (was: "Re: [rfa] function parameter shadowed by local variable in opencl-lang.c")

[Mark Kettenis]

> Date: Mon, 28 Feb 2011 11:39:49 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > That would be great but it produces now 627 errors, I am not going to fix
> > them, they look each one needs a specific fix.
> 
> :-(. Completely impractical at this stage. It's really surprising
> that we would have so many errors of this kind...

Well they're not errors per-se.  Bad style perhaps, but that of course
can be argued.

Re: [rfa] function parameter shadowed by local variable in opencl-lang.c

[Ulrich Weigand]

Joel Brobecker wrote:
> > >Probably this wasn't intentional?
> > >
> > 
> > Oops, another instance of the same thing:
> 
> I wish the compiler would warn us about things like this...  The patch
> looks correct to me, but I will let Ken and Ulrich take a look. Perhaps
> they'll be able to suggest a more specific name than `length2' ;-).

I guess I'd go with something along these lines ...

Bye,
Ulrich


Index: gdb/opencl-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/opencl-lang.c,v
retrieving revision 1.7
diff -u -p -r1.7 opencl-lang.c
--- gdb/opencl-lang.c	21 Feb 2011 15:53:10 -0000	1.7
+++ gdb/opencl-lang.c	28 Feb 2011 15:14:56 -0000
@@ -263,11 +263,11 @@ lval_func_check_validity (const struct v
 
   for (i = start; i < end; i++)
     {
-      int startoffset = (i == start) ? startrest : 0;
-      int length = (i == end) ? endrest : elsize;
+      int comp_offset = (i == start) ? startrest : 0;
+      int comp_length = (i == end) ? endrest : elsize;
 
-      if (!value_bits_valid (c->val, c->indices[i] * elsize + startoffset,
-			     length))
+      if (!value_bits_valid (c->val, c->indices[i] * elsize + comp_offset,
+			     comp_length))
 	return 0;
     }
 
@@ -317,12 +317,12 @@ lval_func_check_synthetic_pointer (const
 
   for (i = start; i < end; i++)
     {
-      int startoffset = (i == start) ? startrest : 0;
-      int length = (i == end) ? endrest : elsize;
+      int comp_offset = (i == start) ? startrest : 0;
+      int comp_length = (i == end) ? endrest : elsize;
 
       if (!value_bits_synthetic_pointer (c->val,
-					 c->indices[i] * elsize + startoffset,
-					 length))
+					 c->indices[i] * elsize + comp_offset,
+					 comp_length))
 	return 0;
     }
 

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFC] Compile GDB with -Wshadow?

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

>> I wish the compiler would warn us about things like this...  The patch
>> looks correct to me, but I will let Ken and Ulrich take a look. Perhaps
>> they'll be able to suggest a more specific name than `length2' ;-).

Joel> Howzabout we add -Wshadow to the list of warning compiler switches
Joel> for GCC? Something like this (untested for now, just to give an idea
Joel> for this discussion)...

-Wshadow also warns about a local variable whose name clashes with a
function.  I find this to be a bit unfriendly.

I wish GCC had a variant of -Wshadow where clashes with functions were
only reported if the shadowing variable had pointer-to-function type.

That said, I don't really mind if we have to change some local variable
names around to satisfy various platforms.  It is just a lot of work.

Tom

Re: [RFC] Compile GDB with -Wshadow?

[Michael Snyder]

Mark Kettenis wrote:
>> Date: Mon, 28 Feb 2011 11:39:49 +0400
>> From: Joel Brobecker <brobecker@adacore.com>
>>
>>> That would be great but it produces now 627 errors, I am not going to fix
>>> them, they look each one needs a specific fix.
>> :-(. Completely impractical at this stage. It's really surprising
>> that we would have so many errors of this kind...
> 
> Well they're not errors per-se.  Bad style perhaps, but that of course
> can be argued.

The ones I pointed out were all shadowing of a parameter.
These are probably shadowing of an outer scope.
As you say, not really errors in either case, but surely
a potential source of confusion for maintainers.

Re: [rfa] function parameter shadowed by local variable in opencl-lang.c

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 442 bytes --]

Ulrich Weigand wrote:
> Joel Brobecker wrote:
>>>> Probably this wasn't intentional?
>>>>
>>> Oops, another instance of the same thing:
>> I wish the compiler would warn us about things like this...  The patch
>> looks correct to me, but I will let Ken and Ulrich take a look. Perhaps
>> they'll be able to suggest a more specific name than `length2' ;-).
> 
> I guess I'd go with something along these lines ...

So committed, as attached.


[-- Attachment #2: rename.txt --]
[-- Type: text/plain, Size: 1512 bytes --]

2011-02-28  Michael Snyder  <msnyder@vmware.com>

	* opencl-lang.c (lval_func_check_validity): Rename inner variables.
	(lval_func_check_synthetic_pointer): Ditto.

Index: opencl-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/opencl-lang.c,v
retrieving revision 1.8
diff -u -p -u -p -r1.8 opencl-lang.c
--- opencl-lang.c	28 Feb 2011 18:31:36 -0000	1.8
+++ opencl-lang.c	28 Feb 2011 18:39:17 -0000
@@ -263,11 +263,11 @@ lval_func_check_validity (const struct v
 
   for (i = start; i < end; i++)
     {
-      int startoffset = (i == start) ? startrest : 0;
-      int length = (i == end) ? endrest : elsize;
+      int comp_offset = (i == start) ? startrest : 0;
+      int comp_length = (i == end) ? endrest : elsize;
 
-      if (!value_bits_valid (c->val, c->indices[i] * elsize + startoffset,
-			     length))
+      if (!value_bits_valid (c->val, c->indices[i] * elsize + comp_offset,
+			     comp_length))
 	return 0;
     }
 
@@ -317,12 +317,12 @@ lval_func_check_synthetic_pointer (const
 
   for (i = start; i < end; i++)
     {
-      int startoffset = (i == start) ? startrest : 0;
-      int length = (i == end) ? endrest : elsize;
+      int comp_offset = (i == start) ? startrest : 0;
+      int comp_length = (i == end) ? endrest : elsize;
 
       if (!value_bits_synthetic_pointer (c->val,
-					 c->indices[i] * elsize + startoffset,
-					 length))
+					 c->indices[i] * elsize + comp_offset,
+					 comp_length))
 	return 0;
     }