| Message ID | 20467491553964233@myt4-c0b480c282c8.qloud-c.yandex.net |
|---|---|
| State | New |
| Headers | show |
| Series | simple sort swap function usage improvements | expand |
On Sat, Mar 30, 2019 at 07:43:53PM +0300, Andrey Abramov wrote: > Replace int type with size_t type of the size argument > in the swap function, also affect all its dependencies. This says _what_ the patch does, but it gives no clue as to _why_ you are doing this. Neither did your 0/5 patch :( Why make this change? Nothing afterward depends on it from what I can tell, so why is it needed? thanks, greg k-h
On Sat, 30 Mar 2019 at 19:38:26 +0100 greh k-h wrote; > On Sat, Mar 30, 2019 at 07:43:53PM +0300, Andrey Abramov wrote: >> Replace int type with size_t type of the size argument >> in the swap function, also affect all its dependencies. > > This says _what_ the patch does, but it gives no clue as to _why_ you > are doing this. Neither did your 0/5 patch :( > > Why make this change? Nothing afterward depends on it from what I can > tell, so why is it needed? It's just a minor cleanup, making things less surprising for future programmers. As I wrote in a comment in my patches, using a signed type for an object size is definitely a wart; ever since C89 it's expected you'd use size_t for the purpose. The connection is that it's a natural consequence of doing a pass over every call site. You're right it could be dropped from the series harmlessly, but it comes from the same work. But it's all of *three* call sites in the kernel which are affected. Surely that's not an unreasonable amount of churn to clean up a wart?
On Sat, Mar 30, 2019 at 08:15:49PM +0000, George Spelvin wrote: > On Sat, 30 Mar 2019 at 19:38:26 +0100 greh k-h wrote; > > On Sat, Mar 30, 2019 at 07:43:53PM +0300, Andrey Abramov wrote: > >> Replace int type with size_t type of the size argument > >> in the swap function, also affect all its dependencies. > > > > This says _what_ the patch does, but it gives no clue as to _why_ you > > are doing this. Neither did your 0/5 patch :( > > > > Why make this change? Nothing afterward depends on it from what I can > > tell, so why is it needed? > > It's just a minor cleanup, making things less surprising for future > programmers. As I wrote in a comment in my patches, using a signed type > for an object size is definitely a wart; ever since C89 it's expected > you'd use size_t for the purpose. You did not say that in this commit log :) > The connection is that it's a natural consequence of doing a pass over > every call site. > > You're right it could be dropped from the series harmlessly, but it > comes from the same work. But it's all of *three* call sites in the kernel > which are affected. Surely that's not an unreasonable amount of churn > to clean up a wart? If you think it is a wart, wonderful, yes, let's fix it up. But again, a changelog comment should explain _why_ a commit is needed, not _what_ it does, as we can see from the diff itself exactly what the commit does. thanks, greg k-h
On Sat, 30 Mar 2019 at 21:24:18 +0100, Greg KH wrote: > On Sat, Mar 30, 2019 at 08:15:49PM +0000, George Spelvin wrote: >> On Sat, 30 Mar 2019 at 19:38:26 +0100 Greh KH wrote; >> > On Sat, Mar 30, 2019 at 07:43:53PM +0300, Andrey Abramov wrote: >>>> Replace int type with size_t type of the size argument >>>> in the swap function, also affect all its dependencies. >>> >>> This says _what_ the patch does, but it gives no clue as to _why_ you >>> are doing this. Neither did your 0/5 patch :( >>> >>> Why make this change? Nothing afterward depends on it from what I can >>> tell, so why is it needed? >> >> It's just a minor cleanup, making things less surprising for future >> programmers. As I wrote in a comment in my patches, using a signed type >> for an object size is definitely a wart; ever since C89 it's expected >> you'd use size_t for the purpose. > > You did not say that in this commit log :) Just to clarify: Not My Patch. I approve, but it's Andrey's patch. Your point is taken that the commit message needs to be improved to explain why. I just answered because it wasn't clear how much of your question was rhetorical. > If you think it is a wart, wonderful, yes, let's fix it up. But again, > a changelog comment should explain _why_ a commit is needed, not _what_ > it does, as we can see from the diff itself exactly what the commit > does. It was so obvious to me that I didn't question it, but you have a good point and I'm sure Andrey can clarify. Thanks for the attention!
30.03.2019, 23:17, "George Spelvin" <lkml@sdf.org>: > On Sat, 30 Mar 2019 at 19:38:26 +0100 greh k-h wrote; >> On Sat, Mar 30, 2019 at 07:43:53PM +0300, Andrey Abramov wrote: >>> Replace int type with size_t type of the size argument >>> in the swap function, also affect all its dependencies. >> >> This says _what_ the patch does, but it gives no clue as to _why_ you >> are doing this. Neither did your 0/5 patch :( >> >> Why make this change? Nothing afterward depends on it from what I can >> tell, so why is it needed? > > It's just a minor cleanup, making things less surprising for future > programmers. As I wrote in a comment in my patches, using a signed type > for an object size is definitely a wart; ever since C89 it's expected > you'd use size_t for the purpose. > > The connection is that it's a natural consequence of doing a pass over > every call site. > > You're right it could be dropped from the series harmlessly, but it > comes from the same work. But it's all of *three* call sites in the kernel > which are affected. Surely that's not an unreasonable amount of churn > to clean up a wart? George Spelvin is absolutely right: "It's just a minor cleanup, making things less surprising for future programmers." 31.03.2019, 00:51, "George Spelvin" <lkml@sdf.org>: > It was so obvious to me that I didn't question it, but you have a > good point and I'm sure Andrey can clarify. Thanks for the attention! I thought that it is obvious enough (argument called "size" should be of type size_t in the 90% of cases). Should I resend this patch with better explanation "why"?
On Sun, Mar 31, 2019 at 10:00:18AM +0300, Andrey Abramov wrote: > 30.03.2019, 23:17, "George Spelvin" <lkml@sdf.org>: > > On Sat, 30 Mar 2019 at 19:38:26 +0100 greh k-h wrote; > >> On Sat, Mar 30, 2019 at 07:43:53PM +0300, Andrey Abramov wrote: > >>> Replace int type with size_t type of the size argument > >>> in the swap function, also affect all its dependencies. > >> > >> This says _what_ the patch does, but it gives no clue as to _why_ you > >> are doing this. Neither did your 0/5 patch :( > >> > >> Why make this change? Nothing afterward depends on it from what I can > >> tell, so why is it needed? > > > > It's just a minor cleanup, making things less surprising for future > > programmers. As I wrote in a comment in my patches, using a signed type > > for an object size is definitely a wart; ever since C89 it's expected > > you'd use size_t for the purpose. > > > > The connection is that it's a natural consequence of doing a pass over > > every call site. > > > > You're right it could be dropped from the series harmlessly, but it > > comes from the same work. But it's all of *three* call sites in the kernel > > which are affected. Surely that's not an unreasonable amount of churn > > to clean up a wart? > > George Spelvin is absolutely right: "It's just a minor cleanup, making > things less surprising for future programmers." Then document it. > 31.03.2019, 00:51, "George Spelvin" <lkml@sdf.org>: > > It was so obvious to me that I didn't question it, but you have a > > good point and I'm sure Andrey can clarify. Thanks for the attention! > > I thought that it is obvious enough (argument called "size" should be > of type size_t in the 90% of cases). Should I resend this patch with > better explanation "why"? Yes, "int" is a very nice variable for "size", you need to explain why it is better to use size_t here please. thanks, greg k-h
From: gregkh@linuxfoundation.org > Sent: 31 March 2019 11:54 ... > Yes, "int" is a very nice variable for "size", you need to explain why > it is better to use size_t here please. Actually, on x86_64 you probably want 'unsigned int' to avoid the compiler having to generate a sign-extending register move if the value is ever used in a 64bit expression (eg an address calculation). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 4/1/19 7:46 AM, David Laight wrote: > From: gregkh@linuxfoundation.org >> Sent: 31 March 2019 11:54 > ... >> Yes, "int" is a very nice variable for "size", you need to explain why >> it is better to use size_t here please. > Actually, on x86_64 you probably want 'unsigned int' to avoid the > compiler having to generate a sign-extending register move if the > value is ever used in a 64bit expression (eg an address calculation). Thats likely true for non x86 arches too (for certain on ARC). That is also the reason I dislike "bool", despite its "software engineering" benefits. Per ARC ABI (and likely others too) it is signed 8 bits and any use thereof, requires the compiler to generate an additional EXTB instruction to promote to 32-bit int with sign extension before using the value. -Vineet
01.04.2019, 21:02, "Vineet Gupta" <vineet.gupta1@synopsys.com>: > On 4/1/19 7:46 AM, David Laight wrote: >> From: gregkh@linuxfoundation.org >>> Sent: 31 March 2019 11:54 >> ... >>> Yes, "int" is a very nice variable for "size", you need to explain why >>> it is better to use size_t here please. >> Actually, on x86_64 you probably want 'unsigned int' to avoid the >> compiler having to generate a sign-extending register move if the >> value is ever used in a 64bit expression (eg an address calculation). > > Thats likely true for non x86 arches too (for certain on ARC). That is also the > reason I dislike "bool", despite its "software engineering" benefits. Per ARC ABI > (and likely others too) it is signed 8 bits and any use thereof, requires the > compiler to generate an additional EXTB instruction to promote to 32-bit int with > sign extension before using the value. > > -Vineet George Spelvin wrote "So how about *deleting* the parameter instead? That simplifies everything.", and he is right, so I am just going to completely remove it. Any objections? -- With Best Regards, Andrey Abramov
On 4/1/19 11:14 AM, Andrey Abramov wrote: > George Spelvin wrote "So how about *deleting* the parameter instead? > That simplifies everything.", and he is right, > so I am just going to completely remove it. > > Any objections? LGTM.
diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c index 89be1be1790c..1078c287198c 100644 --- a/arch/x86/kernel/unwind_orc.c +++ b/arch/x86/kernel/unwind_orc.c @@ -176,7 +176,7 @@ static struct orc_entry *orc_find(unsigned long ip) return orc_ftrace_find(ip); } -static void orc_sort_swap(void *_a, void *_b, int size) +static void orc_sort_swap(void *_a, void *_b, size_t size) { struct orc_entry *orc_a, *orc_b; struct orc_entry orc_tmp; diff --git a/include/linux/sort.h b/include/linux/sort.h index 2b99a5dd073d..aea39d552ff7 100644 --- a/include/linux/sort.h +++ b/include/linux/sort.h @@ -6,6 +6,6 @@ void sort(void *base, size_t num, size_t size, int (*cmp)(const void *, const void *), - void (*swap)(void *, void *, int)); + void (*swap)(void *, void *, size_t)); #endif diff --git a/kernel/jump_label.c b/kernel/jump_label.c index bad96b476eb6..340b788571fb 100644 --- a/kernel/jump_label.c +++ b/kernel/jump_label.c @@ -45,7 +45,7 @@ static int jump_label_cmp(const void *a, const void *b) return 0; } -static void jump_label_swap(void *a, void *b, int size) +static void jump_label_swap(void *a, void *b, size_t size) { long delta = (unsigned long)a - (unsigned long)b; struct jump_entry *jea = a; diff --git a/lib/extable.c b/lib/extable.c index f54996fdd0b8..db2888342cd7 100644 --- a/lib/extable.c +++ b/lib/extable.c @@ -28,7 +28,7 @@ static inline unsigned long ex_to_insn(const struct exception_table_entry *x) #ifndef ARCH_HAS_RELATIVE_EXTABLE #define swap_ex NULL #else -static void swap_ex(void *a, void *b, int size) +static void swap_ex(void *a, void *b, size_t size) { struct exception_table_entry *x = a, *y = b, tmp; int delta = b - a; diff --git a/lib/sort.c b/lib/sort.c index 50855ea8c262..60fbbc29104a 100644 --- a/lib/sort.c +++ b/lib/sort.c @@ -114,7 +114,7 @@ static void swap_bytes(void *a, void *b, size_t n) } while (n); } -typedef void (*swap_func_t)(void *a, void *b, int size); +typedef void (*swap_func_t)(void *a, void *b, size_t size); /* * The values are arbitrary as long as they can't be confused with @@ -138,7 +138,7 @@ static void do_swap(void *a, void *b, size_t size, swap_func_t swap_func) else if (swap_func == SWAP_BYTES) swap_bytes(a, b, size); else - swap_func(a, b, (int)size); + swap_func(a, b, size); } /** @@ -187,7 +187,7 @@ static size_t parent(size_t i, unsigned int lsbit, size_t size) */ void sort(void *base, size_t num, size_t size, int (*cmp_func)(const void *, const void *), - void (*swap_func)(void *, void *, int size)) + void (*swap_func)(void *, void *, size_t size)) { /* pre-scale counters for performance */ size_t n = num * size, a = (num/2) * size;
Replace int type with size_t type of the size argument in the swap function, also affect all its dependencies. Signed-off-by: Andrey Abramov <st5pub@yandex.ru> --- arch/x86/kernel/unwind_orc.c | 2 +- include/linux/sort.h | 2 +- kernel/jump_label.c | 2 +- lib/extable.c | 2 +- lib/sort.c | 6 +++--- 5 files changed, 7 insertions(+), 7 deletions(-)