diff mbox series

scsi: scsi_debug:Use min_t to replace min

Message ID tencent_E821B8DA466472675139402A7A799C7CCC0A@qq.com (mailing list archive)
State Changes Requested
Headers show
Series scsi: scsi_debug:Use min_t to replace min | expand

Commit Message

xiaopeitux@foxmail.com July 26, 2024, 8:55 a.m. UTC 
From: Pei Xiao <xiaopei01@kylinos.cn>

Use min_t to replace min, min_t is a bit fast because min use
twice typeof.
And using min_t is cleaner here since the min/max macros
do a typecheck while min_t()/max_t() to an explicit type cast.

Fixes the below checkpatch warning:
WARNING: min() should probably be min_t()

Fixes: ad620becda43 ("scsi: scsi_debug: Implement GET STREAM STATUS")
Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
---
 drivers/scsi/scsi_debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Bart Van Assche July 26, 2024, 5:17 p.m. UTC | #1 
On 7/26/24 1:55 AM, xiaopeitux@foxmail.com wrote:
> Use min_t to replace min, min_t is a bit fast because min use
> twice typeof.

Have you measured the difference in compilation speed? Is the difference
even measurable? Do we even care about the difference in compilation
speed in this case?

> And using min_t is cleaner here since the min/max macros
> do a typecheck while min_t()/max_t() to an explicit type cast.

In other words, this change makes the code WORSE. I prefer no casts
over explicit casts because introducing explicit casts makes it
impossible for the compiler to perform type checking.

> Fixes the below checkpatch warning:
> WARNING: min() should probably be min_t()

checkpatch is a tool for checking patches and should not be used to
check upstream code. Additionally, please fix checkpatch such that it
does not complain about the code touched by this patch. The code in
checkpatch that recommends to use min_t()/max_t() should only complain
about min()/max() calls that have explicit casts.

Bart.
xiaopeitux@foxmail.com July 29, 2024, 2:56 a.m. UTC | #2 
I am referring to this commit for my modifications: eb3b214c37ed.
Meanwhile,in the same function (resp_get_stream_status) where I made the modifications, 
have already used min_t(u32, alloc_len, sizeof(arr)),
but it's not used in another place, which looks awkward.
--
Thanks.

Pei Xiao


No virus found
		Checked by Hillstone Network AntiVirus
Bart Van Assche July 29, 2024, 4:45 p.m. UTC | #3 
On 7/28/24 7:56 PM, xiaopeitux@foxmail.com wrote:
> I am referring to this commit for my modifications: eb3b214c37ed.

All min_t() / max_t() introduced by that commit remove a cast. Your
patch does not remove any casts.

Bart.
diff mbox series

Patch

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index a9d8a9c62663..bd24ffa68e85 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4886,7 +4886,7 @@  static int resp_get_stream_status(struct scsi_cmnd *scp,
 	}
 	put_unaligned_be32(offset - 8, &h->len); /* PARAMETER DATA LENGTH */
 
-	return fill_from_dev_buffer(scp, arr, min(offset, alloc_len));
+	return fill_from_dev_buffer(scp, arr, min_t(u32, offset, alloc_len));
 }
 
 static int resp_sync_cache(struct scsi_cmnd *scp,