All of lore.kernel.org
 help / color / mirror / Atom feed
* [KJ] [PATCH] Removed unreachable code and fixed one compiler warning
@ 2006-08-28 16:57 ville palo
  2006-08-28 16:59 ` [KJ] [PATCH] Removed unreachable code and fixed one compiler Benoit Boissinot
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: ville palo @ 2006-08-28 16:57 UTC (permalink / raw)
  To: kernel-janitors



There were some unreachable code segments in drivers/cdrom.
This patch removes them and fixes one compiler warning: 
"gscd.c:269: warning: unsigned int format, long unsigned int arg"

Patch is made against 2.6.18-rc5

Signed-off-by Ville Palo <ville.palo@vi64pa.net>

diff --git a/drivers/cdrom/gscd.c b/drivers/cdrom/gscd.c
index fa70824..03884df 100644
--- a/drivers/cdrom/gscd.c
+++ b/drivers/cdrom/gscd.c
@@ -266,7 +266,7 @@ repeat:
 		goto out;
 
 	if (req->cmd != READ) {
-		printk("GSCD: bad cmd %u\n", rq_data_dir(req));
+		printk("GSCD: bad cmd %lu\n", rq_data_dir(req));
 		end_request(req, 0);
 		goto repeat;
 	}
@@ -569,7 +569,6 @@ static void cmd_out(int cmd_type, char *
 							     respo_count,
 							     CD_FRAMESIZE /
 							     2);
-							return;
 						} else {
 							/* read the data to the buffer (byte) */
 
@@ -578,15 +577,13 @@ static void cmd_out(int cmd_type, char *
 							    (respo_buf,
 							     respo_count,
 							     CD_FRAMESIZE);
-							return;
 						}
 					} else {
 						/* read the info to the buffer */
 						cmd_info_in(respo_buf,
 							    respo_count);
-						return;
 					}
-
+					
 					return;
 				}
 			}
diff --git a/drivers/cdrom/isp16.c b/drivers/cdrom/isp16.c
index db0fd9a..3c1b493 100644
--- a/drivers/cdrom/isp16.c
+++ b/drivers/cdrom/isp16.c
@@ -318,7 +318,6 @@ isp16_cdi_config(int base, u_char drive_
 		printk("ISP16: dma 1 cannot be used by cdrom interface,"
 		       " due to conflict with the sound card.\n");
 		return -1;
-		break;
 	case 3:
 		dma_code = ISP16_DMA_3;
 		break;
diff --git a/drivers/cdrom/mcdx.c b/drivers/cdrom/mcdx.c
index dcd1ab6..b0ab77b 100644
--- a/drivers/cdrom/mcdx.c
+++ b/drivers/cdrom/mcdx.c
@@ -622,9 +622,6 @@ static void do_mcdx_request(request_queu
 		}
 		end_request(req, 1);
 		goto again;
-
-		xtrace(REQUEST, "end_request(1)\n");
-		end_request(req, 1);
 	}
 
 	goto again;
@@ -1710,11 +1707,7 @@ static int mcdx_tray_move(struct cdrom_d
 	if (!(stuffp->present & DOOR))
 		return -ENOSYS;
 
-	if (position)		/* 1: eject */
-		return mcdx_talk(stuffp, "\xf6", 1, NULL, 1, 5 * HZ, 3);
-	else			/* 0: close */
-		return mcdx_talk(stuffp, "\xf8", 1, NULL, 1, 5 * HZ, 3);
-	return 1;
+	return mcdx_talk(stuffp, position ? "\xf6" : "\xf8", 1, NULL, 1, 5 * HZ, 3);
 }
 
 static int mcdx_stop(struct s_drive_stuff *stuffp, int tries)
diff --git a/drivers/cdrom/sonycd535.c b/drivers/cdrom/sonycd535.c
index 30ab562..65bdd64 100644
--- a/drivers/cdrom/sonycd535.c
+++ b/drivers/cdrom/sonycd535.c
@@ -1028,7 +1028,6 @@ sony_get_subchnl_info(void __user *arg)
 		if (copy_to_user(arg, &schi, sizeof schi))
 			return -EFAULT;
 		return 0;
-		break;
 
 	case CDROM_AUDIO_INVALID:
 	case CDROM_AUDIO_ERROR:
@@ -1083,7 +1082,6 @@ cdu_ioctl(struct inode *inode,
 			return -EIO;
 		}
 		return 0;
-		break;
 
 	case CDROMSTOP:			/* Spin down the drive */
 		cmd_buff[0] = SONY535_HOLD;
@@ -1103,7 +1101,6 @@ cdu_ioctl(struct inode *inode,
 			return -EIO;
 		}
 		return 0;
-		break;
 
 	case CDROMPAUSE:			/* Pause the drive */
 		cmd_buff[0] = SONY535_HOLD;		/* CDU-31 driver uses AUDIO_STOP, not pause */
@@ -1121,7 +1118,6 @@ cdu_ioctl(struct inode *inode,
 		cur_pos_msf[2] = last_sony_subcode->abs_msf[2];
 		sony_audio_status = CDROM_AUDIO_PAUSED;
 		return 0;
-		break;
 
 	case CDROMRESUME:			/* Start the drive after being paused */
 		set_drive_mode(SONY535_AUDIO_DRIVE_MODE, status);
@@ -1150,7 +1146,6 @@ cdu_ioctl(struct inode *inode,
 		}
 		sony_audio_status = CDROM_AUDIO_PLAY;
 		return 0;
-		break;
 
 	case CDROMPLAYMSF:			/* Play starting at the given MSF address. */
 		if (copy_from_user(params, argp, 6))
@@ -1181,7 +1176,6 @@ cdu_ioctl(struct inode *inode,
 		final_pos_msf[2] = cmd_buff[9];
 		sony_audio_status = CDROM_AUDIO_PLAY;
 		return 0;
-		break;
 
 	case CDROMREADTOCHDR:		/* Read the table of contents header */
 		{
@@ -1197,7 +1191,6 @@ cdu_ioctl(struct inode *inode,
 				return -EFAULT;
 		}
 		return 0;
-		break;
 
 	case CDROMREADTOCENTRY:	/* Read a given table of contents entry */
 		{
@@ -1240,7 +1233,6 @@ cdu_ioctl(struct inode *inode,
 				return -EFAULT;
 		}
 		return 0;
-		break;
 
 	case CDROMPLAYTRKIND:		/* Play a track.  This currently ignores index. */
 		{
@@ -1346,7 +1338,6 @@ cdu_ioctl(struct inode *inode,
 			return -EIO;
 		}
 		return 0;
-		break;
 
 	default:
 		return -EINVAL;


_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [KJ] [PATCH] Removed unreachable code and fixed one compiler
  2006-08-28 16:57 [KJ] [PATCH] Removed unreachable code and fixed one compiler warning ville palo
@ 2006-08-28 16:59 ` Benoit Boissinot
  2006-08-28 17:26 ` Nishanth Aravamudan
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Benoit Boissinot @ 2006-08-28 16:59 UTC (permalink / raw)
  To: kernel-janitors

On 8/28/06, ville palo <ville.palo@vi64pa.net> wrote:
> There were some unreachable code segments in drivers/cdrom.
> This patch removes them and fixes one compiler warning:
> "gscd.c:269: warning: unsigned int format, long unsigned int arg"
>
> Patch is made against 2.6.18-rc5
>
> @@ -1710,11 +1707,7 @@ static int mcdx_tray_move(struct cdrom_d
>         if (!(stuffp->present & DOOR))
>                 return -ENOSYS;
>
> -       if (position)           /* 1: eject */
> -               return mcdx_talk(stuffp, "\xf6", 1, NULL, 1, 5 * HZ, 3);
> -       else                    /* 0: close */
> -               return mcdx_talk(stuffp, "\xf8", 1, NULL, 1, 5 * HZ, 3);
> -       return 1;
> +       return mcdx_talk(stuffp, position ? "\xf6" : "\xf8", 1, NULL, 1, 5 * HZ, 3);
>  }
>
You are removing the comments here, and it is useful information to
understand the code.

regards,

Benoit
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [KJ] [PATCH] Removed unreachable code and fixed one compiler
  2006-08-28 16:57 [KJ] [PATCH] Removed unreachable code and fixed one compiler warning ville palo
  2006-08-28 16:59 ` [KJ] [PATCH] Removed unreachable code and fixed one compiler Benoit Boissinot
@ 2006-08-28 17:26 ` Nishanth Aravamudan
  2006-08-28 17:26 ` [KJ] [PATCH] Removed unreachable code and fixed one ville palo
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Nishanth Aravamudan @ 2006-08-28 17:26 UTC (permalink / raw)
  To: kernel-janitors

On 28.08.2006 [19:57:31 +0300], ville palo wrote:
> 
> 
> There were some unreachable code segments in drivers/cdrom.
> This patch removes them and fixes one compiler warning: 
> "gscd.c:269: warning: unsigned int format, long unsigned int arg"

Should be separate patches then. Please read tpp
(http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt) and
Documentation/SubmittingPatches if you haven't already (reread them, if
you have).

> Patch is made against 2.6.18-rc5

Not necessary in general.

> Signed-off-by Ville Palo <ville.palo@vi64pa.net>

Canonical form requires the colon, please

Signed-off-by: Random J Developer <random@developer.example.org>

> index fa70824..03884df 100644
> --- a/drivers/cdrom/gscd.c
> +++ b/drivers/cdrom/gscd.c
> @@ -266,7 +266,7 @@ repeat:
>  		goto out;
>  
>  	if (req->cmd != READ) {
> -		printk("GSCD: bad cmd %u\n", rq_data_dir(req));
> +		printk("GSCD: bad cmd %lu\n", rq_data_dir(req));
>  		end_request(req, 0);
>  		goto repeat;
>  	}

Just to be clear, this should be a separate patch.

> @@ -578,15 +577,13 @@ static void cmd_out(int cmd_type, char *
>  							    (respo_buf,
>  							     respo_count,
>  							     CD_FRAMESIZE);
> -							return;
>  						}
>  					} else {
>  						/* read the info to the buffer */
>  						cmd_info_in(respo_buf,
>  							    respo_count);
> -						return;
>  					}
> -
> +					

Whitespace insertion?

>  					return;
>  				}
>  			}
> diff --git a/drivers/cdrom/isp16.c b/drivers/cdrom/isp16.c
> index db0fd9a..3c1b493 100644
> --- a/drivers/cdrom/isp16.c
> +++ b/drivers/cdrom/isp16.c
> @@ -318,7 +318,6 @@ isp16_cdi_config(int base, u_char drive_
>  		printk("ISP16: dma 1 cannot be used by cdrom interface,"
>  		       " due to conflict with the sound card.\n");
>  		return -1;
> -		break;
>  	case 3:
>  		dma_code = ISP16_DMA_3;
>  		break;
> diff --git a/drivers/cdrom/mcdx.c b/drivers/cdrom/mcdx.c
> index dcd1ab6..b0ab77b 100644
> --- a/drivers/cdrom/mcdx.c
> +++ b/drivers/cdrom/mcdx.c
> @@ -622,9 +622,6 @@ static void do_mcdx_request(request_queu
>  		}
>  		end_request(req, 1);
>  		goto again;
> -
> -		xtrace(REQUEST, "end_request(1)\n");
> -		end_request(req, 1);

Perhapse move the xtrace (which is presumably useful) to before the
goto?

>  	}
>  
>  	goto again;
> @@ -1710,11 +1707,7 @@ static int mcdx_tray_move(struct cdrom_d
>  	if (!(stuffp->present & DOOR))
>  		return -ENOSYS;
>  
> -	if (position)		/* 1: eject */
> -		return mcdx_talk(stuffp, "\xf6", 1, NULL, 1, 5 * HZ, 3);
> -	else			/* 0: close */
> -		return mcdx_talk(stuffp, "\xf8", 1, NULL, 1, 5 * HZ, 3);
> -	return 1;
> +	return mcdx_talk(stuffp, position ? "\xf6" : "\xf8", 1, NULL, 1, 5 * HZ, 3);

As mentioned elsewhere the comments are useful. This also doesn't seem
to fit into either category of your description and thus should have
been in a different patch (Have you (re)read tpp or SubmittingPatches yet?).

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [KJ] [PATCH] Removed unreachable code and fixed one
  2006-08-28 16:57 [KJ] [PATCH] Removed unreachable code and fixed one compiler warning ville palo
  2006-08-28 16:59 ` [KJ] [PATCH] Removed unreachable code and fixed one compiler Benoit Boissinot
  2006-08-28 17:26 ` Nishanth Aravamudan
@ 2006-08-28 17:26 ` ville palo
  2006-08-28 17:49 ` ville palo
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: ville palo @ 2006-08-28 17:26 UTC (permalink / raw)
  To: kernel-janitors

On Mon, 2006-08-28 at 18:59 +0200, Benoit Boissinot wrote:
> You are removing the comments here, and it is useful information to
> understand the code.
> 

Yes, you are right, here's the fixed patch:


Signed-off-by: Ville Palo <ville.palo@vi64pa.net>

diff --git a/drivers/cdrom/gscd.c b/drivers/cdrom/gscd.c
index fa70824..03884df 100644
--- a/drivers/cdrom/gscd.c
+++ b/drivers/cdrom/gscd.c
@@ -266,7 +266,7 @@ repeat:
 		goto out;
 
 	if (req->cmd != READ) {
-		printk("GSCD: bad cmd %u\n", rq_data_dir(req));
+		printk("GSCD: bad cmd %lu\n", rq_data_dir(req));
 		end_request(req, 0);
 		goto repeat;
 	}
@@ -569,7 +569,6 @@ static void cmd_out(int cmd_type, char *
 							     respo_count,
 							     CD_FRAMESIZE /
 							     2);
-							return;
 						} else {
 							/* read the data to the buffer (byte) */
 
@@ -578,15 +577,13 @@ static void cmd_out(int cmd_type, char *
 							    (respo_buf,
 							     respo_count,
 							     CD_FRAMESIZE);
-							return;
 						}
 					} else {
 						/* read the info to the buffer */
 						cmd_info_in(respo_buf,
 							    respo_count);
-						return;
 					}
-
+					
 					return;
 				}
 			}
diff --git a/drivers/cdrom/isp16.c b/drivers/cdrom/isp16.c
index db0fd9a..3c1b493 100644
--- a/drivers/cdrom/isp16.c
+++ b/drivers/cdrom/isp16.c
@@ -318,7 +318,6 @@ isp16_cdi_config(int base, u_char drive_
 		printk("ISP16: dma 1 cannot be used by cdrom interface,"
 		       " due to conflict with the sound card.\n");
 		return -1;
-		break;
 	case 3:
 		dma_code = ISP16_DMA_3;
 		break;
diff --git a/drivers/cdrom/mcdx.c b/drivers/cdrom/mcdx.c
index dcd1ab6..bcff5a8 100644
--- a/drivers/cdrom/mcdx.c
+++ b/drivers/cdrom/mcdx.c
@@ -622,9 +622,6 @@ static void do_mcdx_request(request_queu
 		}
 		end_request(req, 1);
 		goto again;
-
-		xtrace(REQUEST, "end_request(1)\n");
-		end_request(req, 1);
 	}
 
 	goto again;
@@ -1710,11 +1707,8 @@ static int mcdx_tray_move(struct cdrom_d
 	if (!(stuffp->present & DOOR))
 		return -ENOSYS;
 
-	if (position)		/* 1: eject */
-		return mcdx_talk(stuffp, "\xf6", 1, NULL, 1, 5 * HZ, 3);
-	else			/* 0: close */
-		return mcdx_talk(stuffp, "\xf8", 1, NULL, 1, 5 * HZ, 3);
-	return 1;
+	/* If position is 1 then eject, otherwise close */
+	return mcdx_talk(stuffp, position ? "\xf6" : "\xf8", 1, NULL, 1, 5 * HZ, 3);
 }
 
 static int mcdx_stop(struct s_drive_stuff *stuffp, int tries)
diff --git a/drivers/cdrom/sonycd535.c b/drivers/cdrom/sonycd535.c
index 30ab562..65bdd64 100644
--- a/drivers/cdrom/sonycd535.c
+++ b/drivers/cdrom/sonycd535.c
@@ -1028,7 +1028,6 @@ sony_get_subchnl_info(void __user *arg)
 		if (copy_to_user(arg, &schi, sizeof schi))
 			return -EFAULT;
 		return 0;
-		break;
 
 	case CDROM_AUDIO_INVALID:
 	case CDROM_AUDIO_ERROR:
@@ -1083,7 +1082,6 @@ cdu_ioctl(struct inode *inode,
 			return -EIO;
 		}
 		return 0;
-		break;
 
 	case CDROMSTOP:			/* Spin down the drive */
 		cmd_buff[0] = SONY535_HOLD;
@@ -1103,7 +1101,6 @@ cdu_ioctl(struct inode *inode,
 			return -EIO;
 		}
 		return 0;
-		break;
 
 	case CDROMPAUSE:			/* Pause the drive */
 		cmd_buff[0] = SONY535_HOLD;		/* CDU-31 driver uses AUDIO_STOP, not pause */
@@ -1121,7 +1118,6 @@ cdu_ioctl(struct inode *inode,
 		cur_pos_msf[2] = last_sony_subcode->abs_msf[2];
 		sony_audio_status = CDROM_AUDIO_PAUSED;
 		return 0;
-		break;
 
 	case CDROMRESUME:			/* Start the drive after being paused */
 		set_drive_mode(SONY535_AUDIO_DRIVE_MODE, status);
@@ -1150,7 +1146,6 @@ cdu_ioctl(struct inode *inode,
 		}
 		sony_audio_status = CDROM_AUDIO_PLAY;
 		return 0;
-		break;
 
 	case CDROMPLAYMSF:			/* Play starting at the given MSF address. */
 		if (copy_from_user(params, argp, 6))
@@ -1181,7 +1176,6 @@ cdu_ioctl(struct inode *inode,
 		final_pos_msf[2] = cmd_buff[9];
 		sony_audio_status = CDROM_AUDIO_PLAY;
 		return 0;
-		break;
 
 	case CDROMREADTOCHDR:		/* Read the table of contents header */
 		{
@@ -1197,7 +1191,6 @@ cdu_ioctl(struct inode *inode,
 				return -EFAULT;
 		}
 		return 0;
-		break;
 
 	case CDROMREADTOCENTRY:	/* Read a given table of contents entry */
 		{
@@ -1240,7 +1233,6 @@ cdu_ioctl(struct inode *inode,
 				return -EFAULT;
 		}
 		return 0;
-		break;
 
 	case CDROMPLAYTRKIND:		/* Play a track.  This currently ignores index. */
 		{
@@ -1346,7 +1338,6 @@ cdu_ioctl(struct inode *inode,
 			return -EIO;
 		}
 		return 0;
-		break;
 
 	default:
 		return -EINVAL;


_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [KJ] [PATCH] Removed unreachable code and fixed one
  2006-08-28 16:57 [KJ] [PATCH] Removed unreachable code and fixed one compiler warning ville palo
                   ` (2 preceding siblings ...)
  2006-08-28 17:26 ` [KJ] [PATCH] Removed unreachable code and fixed one ville palo
@ 2006-08-28 17:49 ` ville palo
  2006-08-28 17:51 ` [KJ] [PATCH] Removed unreachable code and fixed one compiler Nishanth Aravamudan
  2006-08-28 18:00 ` Richard Knutsson
  5 siblings, 0 replies; 7+ messages in thread
From: ville palo @ 2006-08-28 17:49 UTC (permalink / raw)
  To: kernel-janitors

On Mon, 2006-08-28 at 10:26 -0700, Nishanth Aravamudan wrote:
> On 28.08.2006 [19:57:31 +0300], ville palo wrote:
> > 
> > 
> > There were some unreachable code segments in drivers/cdrom.
> > This patch removes them and fixes one compiler warning: 
> > "gscd.c:269: warning: unsigned int format, long unsigned int arg"
> 
> Should be separate patches then. Please read tpp
> (http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt) and
> Documentation/SubmittingPatches if you haven't already (reread them, if
> you have).

Ok, I'll reread them :)

> Perhapse move the xtrace (which is presumably useful) to before the
> goto?
> 
> >  	}
> >  
> >  	goto again;
> > @@ -1710,11 +1707,7 @@ static int mcdx_tray_move(struct cdrom_d
> >  	if (!(stuffp->present & DOOR))
> >  		return -ENOSYS;
> >  
> > -	if (position)		/* 1: eject */
> > -		return mcdx_talk(stuffp, "\xf6", 1, NULL, 1, 5 * HZ, 3);
> > -	else			/* 0: close */
> > -		return mcdx_talk(stuffp, "\xf8", 1, NULL, 1, 5 * HZ, 3);
> > -	return 1;
> > +	return mcdx_talk(stuffp, position ? "\xf6" : "\xf8", 1, NULL, 1, 5 * HZ, 3);
> 
> As mentioned elsewhere the comments are useful. This also doesn't seem
> to fit into either category of your description and thus should have
> been in a different patch (Have you (re)read tpp or SubmittingPatches yet?).

I already fixed the comments part but I have to disagree about the
description part. IMHO this fits in "removed unreachable code" category,
because 'return 1;' was unreachable.


_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [KJ] [PATCH] Removed unreachable code and fixed one compiler
  2006-08-28 16:57 [KJ] [PATCH] Removed unreachable code and fixed one compiler warning ville palo
                   ` (3 preceding siblings ...)
  2006-08-28 17:49 ` ville palo
@ 2006-08-28 17:51 ` Nishanth Aravamudan
  2006-08-28 18:00 ` Richard Knutsson
  5 siblings, 0 replies; 7+ messages in thread
From: Nishanth Aravamudan @ 2006-08-28 17:51 UTC (permalink / raw)
  To: kernel-janitors

On 28.08.2006 [20:49:58 +0300], ville palo wrote:
> On Mon, 2006-08-28 at 10:26 -0700, Nishanth Aravamudan wrote:
> > On 28.08.2006 [19:57:31 +0300], ville palo wrote:
> > > 
> > > 
> > > There were some unreachable code segments in drivers/cdrom.
> > > This patch removes them and fixes one compiler warning: 
> > > "gscd.c:269: warning: unsigned int format, long unsigned int arg"
> > 
> > Should be separate patches then. Please read tpp
> > (http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt) and
> > Documentation/SubmittingPatches if you haven't already (reread them, if
> > you have).
> 
> Ok, I'll reread them :)
> 
> > Perhapse move the xtrace (which is presumably useful) to before the
> > goto?
> > 
> > >  	}
> > >  
> > >  	goto again;
> > > @@ -1710,11 +1707,7 @@ static int mcdx_tray_move(struct cdrom_d
> > >  	if (!(stuffp->present & DOOR))
> > >  		return -ENOSYS;
> > >  
> > > -	if (position)		/* 1: eject */
> > > -		return mcdx_talk(stuffp, "\xf6", 1, NULL, 1, 5 * HZ, 3);
> > > -	else			/* 0: close */
> > > -		return mcdx_talk(stuffp, "\xf8", 1, NULL, 1, 5 * HZ, 3);
> > > -	return 1;
> > > +	return mcdx_talk(stuffp, position ? "\xf6" : "\xf8", 1, NULL, 1, 5 * HZ, 3);
> > 
> > As mentioned elsewhere the comments are useful. This also doesn't seem
> > to fit into either category of your description and thus should have
> > been in a different patch (Have you (re)read tpp or SubmittingPatches yet?).
> 
> I already fixed the comments part but I have to disagree about the
> description part. IMHO this fits in "removed unreachable code"
> category, because 'return 1;' was unreachable.

Only the return 1; removal does. The other is code rearrangement, and
is a distinct (and unnecessary) logical change.

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [KJ] [PATCH] Removed unreachable code and fixed one compiler
  2006-08-28 16:57 [KJ] [PATCH] Removed unreachable code and fixed one compiler warning ville palo
                   ` (4 preceding siblings ...)
  2006-08-28 17:51 ` [KJ] [PATCH] Removed unreachable code and fixed one compiler Nishanth Aravamudan
@ 2006-08-28 18:00 ` Richard Knutsson
  5 siblings, 0 replies; 7+ messages in thread
From: Richard Knutsson @ 2006-08-28 18:00 UTC (permalink / raw)
  To: kernel-janitors

ville palo wrote:

>On Mon, 2006-08-28 at 18:59 +0200, Benoit Boissinot wrote:
>  
>
>>You are removing the comments here, and it is useful information to
>>understand the code.
>>
>>    
>>
>
>Yes, you are right, here's the fixed patch:
>
>
>Signed-off-by: Ville Palo <ville.palo@vi64pa.net>
>
>  
>
<slice>

> 	goto again;
>@@ -1710,11 +1707,8 @@ static int mcdx_tray_move(struct cdrom_d
> 	if (!(stuffp->present & DOOR))
> 		return -ENOSYS;
> 
>-	if (position)		/* 1: eject */
>-		return mcdx_talk(stuffp, "\xf6", 1, NULL, 1, 5 * HZ, 3);
>-	else			/* 0: close */
>-		return mcdx_talk(stuffp, "\xf8", 1, NULL, 1, 5 * HZ, 3);
>-	return 1;
>+	/* If position is 1 then eject, otherwise close */
>+	return mcdx_talk(stuffp, position ? "\xf6" : "\xf8", 1, NULL, 1, 5 * HZ, 3);
> }
> 
>
Not to be picky, but isn't it: "If position is 0 then close, otherwise 
eject"? ;)

Richard Knutsson

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2006-08-28 18:00 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-08-28 16:57 [KJ] [PATCH] Removed unreachable code and fixed one compiler warning ville palo
2006-08-28 16:59 ` [KJ] [PATCH] Removed unreachable code and fixed one compiler Benoit Boissinot
2006-08-28 17:26 ` Nishanth Aravamudan
2006-08-28 17:26 ` [KJ] [PATCH] Removed unreachable code and fixed one ville palo
2006-08-28 17:49 ` ville palo
2006-08-28 17:51 ` [KJ] [PATCH] Removed unreachable code and fixed one compiler Nishanth Aravamudan
2006-08-28 18:00 ` Richard Knutsson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.