Re: [lm-sensors] [PATCH 1/4] kernel.h: add find_closest() macro
On Wed, Feb 25, 2015 at 11:59:51AM +0100, Bartosz Golaszewski wrote: > 2015-02-24 21:51 GMT+01:00 Guenter Roeck : > > I think the lm85 conversion actually introduces a bug with such an > > off-by-one mistake. And if it doesn't, there is still a unexplained > > and not easy to understand '-1' in one of the calls to find_closest(). > > > > So the question is if the new code really improves the situation in that > > respect. > > Yes, it's a mistake. I couldn't test this one and missed this '-1'. > Sorry for that. > > As for the size comparisons - at first glance it seems as if it adds bloat: > > ina2xx: > add/remove: 0/0 grow/shrink: 1/0 up/down: 24/0 (24) > function old new delta > ina226_set_interval 296 320 +24 > > lm85: > add/remove: 0/0 grow/shrink: 3/0 up/down: 72/0 (72) > function old new delta > set_temp_auto_temp_min 364 388 +24 > set_temp_auto_temp_max 336 360 +24 > set_pwm_freq 284 308 +24 > > w83795: > add/remove: 0/0 grow/shrink: 1/0 up/down: 16/0 (16) > function old new delta > store_pwm496 512 +16 > > But this actually comes from DIV_ROUND_CLOSEST() since replacing it > with a simple '/ 2' gives different results: > > ina2xx.ko: > add/remove: 0/0 grow/shrink: 1/0 up/down: 6/0 (6) > function old new delta > __UNIQUE_ID_vermagic0 73 79 +6 > > lm85.ko: > add/remove: 0/0 grow/shrink: 1/0 up/down: 6/0 (6) > function old new delta > __UNIQUE_ID_vermagic0 73 79 +6 > > w83795.ko: > add/remove: 0/0 grow/shrink: 2/0 up/down: 14/0 (14) > function old new delta > store_pwm496 504 +8 > __UNIQUE_ID_vermagic0 73 79 +6 > > The idea however was to remove duplicated operations from source files > and prevent off-by-one bugs (how ironic the lm85 patch...), not really > to reduce size. > I don't know, seems to me it can cause more trouble and potential for getting something wrong than it is worth. I'll definitely want to see feedback (and acceptance) from more senior maintainers than myself. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [PATCH 1/4] kernel.h: add find_closest() macro
2015-02-24 21:51 GMT+01:00 Guenter Roeck : > I think the lm85 conversion actually introduces a bug with such an > off-by-one mistake. And if it doesn't, there is still a unexplained > and not easy to understand '-1' in one of the calls to find_closest(). > > So the question is if the new code really improves the situation in that > respect. Yes, it's a mistake. I couldn't test this one and missed this '-1'. Sorry for that. As for the size comparisons - at first glance it seems as if it adds bloat: ina2xx: add/remove: 0/0 grow/shrink: 1/0 up/down: 24/0 (24) function old new delta ina226_set_interval 296 320 +24 lm85: add/remove: 0/0 grow/shrink: 3/0 up/down: 72/0 (72) function old new delta set_temp_auto_temp_min 364 388 +24 set_temp_auto_temp_max 336 360 +24 set_pwm_freq 284 308 +24 w83795: add/remove: 0/0 grow/shrink: 1/0 up/down: 16/0 (16) function old new delta store_pwm496 512 +16 But this actually comes from DIV_ROUND_CLOSEST() since replacing it with a simple '/ 2' gives different results: ina2xx.ko: add/remove: 0/0 grow/shrink: 1/0 up/down: 6/0 (6) function old new delta __UNIQUE_ID_vermagic0 73 79 +6 lm85.ko: add/remove: 0/0 grow/shrink: 1/0 up/down: 6/0 (6) function old new delta __UNIQUE_ID_vermagic0 73 79 +6 w83795.ko: add/remove: 0/0 grow/shrink: 2/0 up/down: 14/0 (14) function old new delta store_pwm496 504 +8 __UNIQUE_ID_vermagic0 73 79 +6 The idea however was to remove duplicated operations from source files and prevent off-by-one bugs (how ironic the lm85 patch...), not really to reduce size. Bart -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [PATCH 1/4] kernel.h: add find_closest() macro
On Wed, Feb 25, 2015 at 11:59:51AM +0100, Bartosz Golaszewski wrote: 2015-02-24 21:51 GMT+01:00 Guenter Roeck li...@roeck-us.net: I think the lm85 conversion actually introduces a bug with such an off-by-one mistake. And if it doesn't, there is still a unexplained and not easy to understand '-1' in one of the calls to find_closest(). So the question is if the new code really improves the situation in that respect. Yes, it's a mistake. I couldn't test this one and missed this '-1'. Sorry for that. As for the size comparisons - at first glance it seems as if it adds bloat: ina2xx: add/remove: 0/0 grow/shrink: 1/0 up/down: 24/0 (24) function old new delta ina226_set_interval 296 320 +24 lm85: add/remove: 0/0 grow/shrink: 3/0 up/down: 72/0 (72) function old new delta set_temp_auto_temp_min 364 388 +24 set_temp_auto_temp_max 336 360 +24 set_pwm_freq 284 308 +24 w83795: add/remove: 0/0 grow/shrink: 1/0 up/down: 16/0 (16) function old new delta store_pwm496 512 +16 But this actually comes from DIV_ROUND_CLOSEST() since replacing it with a simple '/ 2' gives different results: ina2xx.ko: add/remove: 0/0 grow/shrink: 1/0 up/down: 6/0 (6) function old new delta __UNIQUE_ID_vermagic0 73 79 +6 lm85.ko: add/remove: 0/0 grow/shrink: 1/0 up/down: 6/0 (6) function old new delta __UNIQUE_ID_vermagic0 73 79 +6 w83795.ko: add/remove: 0/0 grow/shrink: 2/0 up/down: 14/0 (14) function old new delta store_pwm496 504 +8 __UNIQUE_ID_vermagic0 73 79 +6 The idea however was to remove duplicated operations from source files and prevent off-by-one bugs (how ironic the lm85 patch...), not really to reduce size. I don't know, seems to me it can cause more trouble and potential for getting something wrong than it is worth. I'll definitely want to see feedback (and acceptance) from more senior maintainers than myself. Guenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [PATCH 1/4] kernel.h: add find_closest() macro
2015-02-24 21:51 GMT+01:00 Guenter Roeck li...@roeck-us.net: I think the lm85 conversion actually introduces a bug with such an off-by-one mistake. And if it doesn't, there is still a unexplained and not easy to understand '-1' in one of the calls to find_closest(). So the question is if the new code really improves the situation in that respect. Yes, it's a mistake. I couldn't test this one and missed this '-1'. Sorry for that. As for the size comparisons - at first glance it seems as if it adds bloat: ina2xx: add/remove: 0/0 grow/shrink: 1/0 up/down: 24/0 (24) function old new delta ina226_set_interval 296 320 +24 lm85: add/remove: 0/0 grow/shrink: 3/0 up/down: 72/0 (72) function old new delta set_temp_auto_temp_min 364 388 +24 set_temp_auto_temp_max 336 360 +24 set_pwm_freq 284 308 +24 w83795: add/remove: 0/0 grow/shrink: 1/0 up/down: 16/0 (16) function old new delta store_pwm496 512 +16 But this actually comes from DIV_ROUND_CLOSEST() since replacing it with a simple '/ 2' gives different results: ina2xx.ko: add/remove: 0/0 grow/shrink: 1/0 up/down: 6/0 (6) function old new delta __UNIQUE_ID_vermagic0 73 79 +6 lm85.ko: add/remove: 0/0 grow/shrink: 1/0 up/down: 6/0 (6) function old new delta __UNIQUE_ID_vermagic0 73 79 +6 w83795.ko: add/remove: 0/0 grow/shrink: 2/0 up/down: 14/0 (14) function old new delta store_pwm496 504 +8 __UNIQUE_ID_vermagic0 73 79 +6 The idea however was to remove duplicated operations from source files and prevent off-by-one bugs (how ironic the lm85 patch...), not really to reduce size. Bart -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [PATCH 1/4] kernel.h: add find_closest() macro
On Tue, Feb 24, 2015 at 12:33:06PM -0800, Phil Pokorny wrote: > On Tue, Feb 24, 2015 at 9:42 AM, Bartosz Golaszewski > wrote: > > > > Searching for the member of an array closest to 'x' is > > duplicated in several places. > > > > Add two macros that implement this algorithm for arrays > > sorted both in ascending and descending order. > > I don't see the point here. You're not saving any code because your > macros create functions at each invocation site. And your macro is > more complicated than the code it replaces because it has all the > syntactic cruft to make it adaptable to the different datatypes and > sort orders. > > Certainly it is easy to make an off by one mistake in a loop like this > so there might be some small value there, but I'm not sure the > complication is worth that savings for the small number of use points. > Particularly because you're not saving any code. > I think the lm85 conversion actually introduces a bug with such an off-by-one mistake. And if it doesn't, there is still a unexplained and not easy to understand '-1' in one of the calls to find_closest(). So the question is if the new code really improves the situation in that respect. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [PATCH 1/4] kernel.h: add find_closest() macro
On Tue, Feb 24, 2015 at 9:42 AM, Bartosz Golaszewski wrote: > > Searching for the member of an array closest to 'x' is > duplicated in several places. > > Add two macros that implement this algorithm for arrays > sorted both in ascending and descending order. I don't see the point here. You're not saving any code because your macros create functions at each invocation site. And your macro is more complicated than the code it replaces because it has all the syntactic cruft to make it adaptable to the different datatypes and sort orders. Certainly it is easy to make an off by one mistake in a loop like this so there might be some small value there, but I'm not sure the complication is worth that savings for the small number of use points. Particularly because you're not saving any code. -- Philip Pokorny, RHCE Chief Technology Officer PENGUIN COMPUTING, Inc www.penguincomputing.com Changing the world through technical innovation -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [PATCH 1/4] kernel.h: add find_closest() macro
On Tue, Feb 24, 2015 at 12:33:06PM -0800, Phil Pokorny wrote: On Tue, Feb 24, 2015 at 9:42 AM, Bartosz Golaszewski bgolaszew...@baylibre.com wrote: Searching for the member of an array closest to 'x' is duplicated in several places. Add two macros that implement this algorithm for arrays sorted both in ascending and descending order. I don't see the point here. You're not saving any code because your macros create functions at each invocation site. And your macro is more complicated than the code it replaces because it has all the syntactic cruft to make it adaptable to the different datatypes and sort orders. Certainly it is easy to make an off by one mistake in a loop like this so there might be some small value there, but I'm not sure the complication is worth that savings for the small number of use points. Particularly because you're not saving any code. I think the lm85 conversion actually introduces a bug with such an off-by-one mistake. And if it doesn't, there is still a unexplained and not easy to understand '-1' in one of the calls to find_closest(). So the question is if the new code really improves the situation in that respect. Guenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [PATCH 1/4] kernel.h: add find_closest() macro
On Tue, Feb 24, 2015 at 9:42 AM, Bartosz Golaszewski bgolaszew...@baylibre.com wrote: Searching for the member of an array closest to 'x' is duplicated in several places. Add two macros that implement this algorithm for arrays sorted both in ascending and descending order. I don't see the point here. You're not saving any code because your macros create functions at each invocation site. And your macro is more complicated than the code it replaces because it has all the syntactic cruft to make it adaptable to the different datatypes and sort orders. Certainly it is easy to make an off by one mistake in a loop like this so there might be some small value there, but I'm not sure the complication is worth that savings for the small number of use points. Particularly because you're not saving any code. -- Philip Pokorny, RHCE Chief Technology Officer PENGUIN COMPUTING, Inc www.penguincomputing.com Changing the world through technical innovation -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
