Skip to contentSkip to conversation

Make the kernel -Wshadow clean ?

RFC PATCH In review Jesper Juhl 3 participants 4 messages Dec 14, 2005

Subsystems (2)
Memory management USB
lore
Share & tools
lore

Jesper Juhl

author
#0 lore raw

RFC PATCH 1/1 Make the kernel -Wshadow clean ?

Hi,

Variables shadowing eachother have a nasty potential to contain hard to discover bugs, so I propose we clean that up.

There are *lots* of cases of variable shadowing in the kernel and real bugs could be lurking in there.

I'm full willing to audit every single case and clean them up, but before I begin such a task I want to "test the waters".

So to get things started I give you the patch below that cleans up

784 counts of
include/linux/jiffies.h:331: warning: declaration of 'jiffies' shadows a global declaration

784 counts of
include/linux/jiffies.h:369: warning: declaration of 'jiffies' shadows a global declaration

30 counts of
include/linux/usb.h:877: warning: declaration of 'complete' shadows a global declaration

30 counts of
include/linux/usb.h:908: warning: declaration of 'complete' shadows a global declaration

30 counts of
include/linux/usb.h:943: warning: declaration of 'complete' shadows a global declaration

1 count of
mm/swap.c:42: warning: declaration of 'page' shadows a parameter

So with just one simple patch we've nuked 1659 -Wshadow warnings out of a total of 2403 that this build of mine generated with my regular .config Of course I picked some of the easy targets to start with, a lot of the remaining ones are cases of 1-10 identical warnings generated by one instance.

Anyway, I think this is a worthwhile task and I'm willing to do it and we may even discover/fix real bugs along the way, so what do you say?

Comments are very welcome as always, and feel free to merge this patch as a starting point if you agree that this is something that ought to be done.

Signed-off-by Jesper Juhl <[email protected]>

---

 include/linux/jiffies.h |    8 ++++----
 include/linux/usb.h     |   12 ++++++------
 mm/swap.c               |    2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)
include/linux/jiffies.h +4 -4
@@ -328,13 +328,13 @@ timespec_to_jiffies(const struct timespe
 }
 
 static __inline__ void
-jiffies_to_timespec(const unsigned long jiffies, struct timespec *value)
+jiffies_to_timespec(const unsigned long jiff, struct timespec *value)
 {
 	/*
 	 * Convert jiffies to nanoseconds and separate with
 	 * one divide.
 	 */
-	u64 nsec = (u64)jiffies * TICK_NSEC;
+	u64 nsec = (u64)jiff * TICK_NSEC;
 	value->tv_sec = div_long_long_rem(nsec, NSEC_PER_SEC, &value->tv_nsec);
 }
 

@@ -366,13 +366,13 @@ timeval_to_jiffies(const struct timeval 
 }
 
 static __inline__ void
-jiffies_to_timeval(const unsigned long jiffies, struct timeval *value)
+jiffies_to_timeval(const unsigned long jiff, struct timeval *value)
 {
 	/*
 	 * Convert jiffies to nanoseconds and separate with
 	 * one divide.
 	 */
-	u64 nsec = (u64)jiffies * TICK_NSEC;
+	u64 nsec = (u64)jiff * TICK_NSEC;
 	value->tv_sec = div_long_long_rem(nsec, NSEC_PER_SEC, &value->tv_usec);
 	value->tv_usec /= NSEC_PER_USEC;
 }
include/linux/usb.h +6 -6
@@ -874,7 +874,7 @@ static inline void usb_fill_control_urb 
 					 unsigned char *setup_packet,
 					 void *transfer_buffer,
 					 int buffer_length,
-					 usb_complete_t complete,
+					 usb_complete_t comp,
 					 void *context)
 {
 	spin_lock_init(&urb->lock);

@@ -883,7 +883,7 @@ static inline void usb_fill_control_urb 
 	urb->setup_packet = setup_packet;
 	urb->transfer_buffer = transfer_buffer;
 	urb->transfer_buffer_length = buffer_length;
-	urb->complete = complete;
+	urb->complete = comp;
 	urb->context = context;
 }
 

@@ -905,7 +905,7 @@ static inline void usb_fill_bulk_urb (st
 				      unsigned int pipe,
 				      void *transfer_buffer,
 				      int buffer_length,
-				      usb_complete_t complete,
+				      usb_complete_t comp,
 				      void *context)
 {
 	spin_lock_init(&urb->lock);

@@ -913,7 +913,7 @@ static inline void usb_fill_bulk_urb (st
 	urb->pipe = pipe;
 	urb->transfer_buffer = transfer_buffer;
 	urb->transfer_buffer_length = buffer_length;
-	urb->complete = complete;
+	urb->complete = comp;
 	urb->context = context;
 }
 

@@ -940,7 +940,7 @@ static inline void usb_fill_int_urb (str
 				     unsigned int pipe,
 				     void *transfer_buffer,
 				     int buffer_length,
-				     usb_complete_t complete,
+				     usb_complete_t comp,
 				     void *context,
 				     int interval)
 {

@@ -949,7 +949,7 @@ static inline void usb_fill_int_urb (str
 	urb->pipe = pipe;
 	urb->transfer_buffer = transfer_buffer;
 	urb->transfer_buffer_length = buffer_length;
-	urb->complete = complete;
+	urb->complete = comp;
 	urb->context = context;
 	if (dev->speed == USB_SPEED_HIGH)
 		urb->interval = 1 << (interval - 1);
mm/swap.c +1 -1
@@ -39,7 +39,7 @@ void put_page(struct page *page)
 	if (unlikely(PageCompound(page))) {
 		page = (struct page *)page_private(page);
 		if (put_page_testzero(page)) {
-			void (*dtor)(struct page *page);
+			void (*dtor)(struct page *pge);
 
 			dtor = (void (*)(struct page *))page[1].mapping;
 			(*dtor)(page);
#1 lore raw

mm/swap.c void put_page(struct page *page) · line 42

Quoted 2 lines · Jesper Juhl
mm/swap.c +1 -1
-			void (*dtor)(struct page *page);
+			void (*dtor)(struct page *pge);

Note that this one just needs to be:

			void (*dtor)(struct page *);
Signature · 3 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core
author
#2 lore raw
Quoted 7 lines · Russell King

mm/swap.c void put_page(struct page *page) · line 42

Quoted 2 lines · Jesper Juhl
mm/swap.c +1 -1
-                     void (*dtor)(struct page *page);
+                     void (*dtor)(struct page *pge);

Note that this one just needs to be:

                        void (*dtor)(struct page *);

Yes, I considered just doing that, but was unsure if the name had been used for some obscure readability or grep'ability reason, so I opted to leave the name but just change it.. In future cases like this one (if people actually want these patches) I'll just nuke the name.

--
Jesper Juhl <[email protected]>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html

#3 lore raw
In reply to Russell King
Quoted 6 lines · Russell King

mm/swap.c void put_page(struct page *page) · line 42

Quoted 2 lines · Jesper Juhl
mm/swap.c +1 -1
-			void (*dtor)(struct page *page);
+			void (*dtor)(struct page *pge);

Note that this one just needs to be:

			void (*dtor)(struct page *);

I would rather see <shadowed> names there.

Signature · 1 line
~Randy