| Message ID | 20220119072450.2890107-1-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | lib/string_helpers: Add a few string helpers | expand |
On Tue, 18 Jan 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > Add some helpers under lib/string_helpers.h so they can be used > throughout the kernel. When I started doing this there were 2 other > previous attempts I know of, not counting the iterations each of them > had: > > 1) https://lore.kernel.org/all/20191023131308.9420-1-jani.nikula@intel.com/ > 2) https://lore.kernel.org/all/20210215142137.64476-1-andriy.shevchenko@linux.intel.com/#t > > Going through the comments I tried to find some common ground and > justification for what is in here, addressing some of the concerns > raised. > > a. This version should be a drop-in replacement for what is currently in > the tree, with no change in behavior or binary size. For binary > size what I checked wat that the linked objects in the end have the > same size (gcc 11). From comments in the previous attempts this seems > also the case for earlier compiler versions > > b. I didn't change the function name to choice_* as suggested by Andrew > Morton in 20191023155619.43e0013f0c8c673a5c508c1e@linux-foundation.org > because other people argumented in favor of shorter names for these > simple helpers - if they are long and people simply not use due to > that, we failed > > c. Use string_helper.h for these helpers - pulling string.h in the > compilations units was one of the concerns and I think re-using this > already existing header is better than creating a new string-choice.h > > d. This doesn't bring onoff() helper as there are some places in the > kernel with onoff as variable - another name is probably needed for > this function in order not to shadow the variable, or those variables > could be renamed. Or if people wanting <someprefix> > try to find a short one > > e. One alternative to all of this suggested by Christian König > (43456ba7-c372-84cc-4949-dcb817188e21@amd.com) would be to add a > printk format. But besides the comment, he also seemed to like > the common function. This brought the argument from others that the > simple yesno()/enabledisable() already used in the code is easier to > remember and use than e.g. %py[DOY] > > Last patch also has some additional conversion of open coded cases. I > preferred starting with drm/ since this is "closer to home". > > I hope this is a good summary of the previous attempts and a way we can > move forward. Thanks for picking this up again. I agree with the approach here. Acked-by: Jani Nikula <jani.nikula@intel.com> > > Andrew Morton, Petr Mladek, Andy Shevchenko: if this is accepted, my > proposal is to take first 2 patches either through mm tree or maybe > vsprintf. Last patch can be taken later through drm. > > thanks > Lucas De Marchi > > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Ben Skeggs <bskeggs@redhat.com> > Cc: Christian König <christian.koenig@amd.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: David Airlie <airlied@linux.ie> > Cc: David S. Miller <davem@davemloft.net> > Cc: Emma Anholt <emma@anholt.net> > Cc: Eryk Brol <eryk.brol@amd.com> > Cc: Francis Laniel <laniel_francis@privacyrequired.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Harry Wentland <harry.wentland@amd.com> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Julia Lawall <julia.lawall@lip6.fr> > Cc: Kentaro Takeda <takedakn@nttdata.co.jp> > Cc: Leo Li <sunpeng.li@amd.com> > Cc: Mikita Lipski <mikita.lipski@amd.com> > Cc: Petr Mladek <pmladek@suse.com> > Cc: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com> > Cc: Raju Rangoju <rajur@chelsio.com> > Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Vishal Kulkarni <vishal@chelsio.com> > > Lucas De Marchi (3): > lib/string_helpers: Consolidate yesno() implementation > lib/string_helpers: Add helpers for enable[d]/disable[d] > drm: Convert open yes/no strings to yesno() > > drivers/gpu/drm/amd/amdgpu/atom.c | 3 ++- > .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 6 +----- > drivers/gpu/drm/drm_client_modeset.c | 3 ++- > drivers/gpu/drm/drm_dp_helper.c | 3 ++- > drivers/gpu/drm/drm_gem.c | 3 ++- > drivers/gpu/drm/i915/i915_utils.h | 15 --------------- > drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c | 4 +++- > drivers/gpu/drm/radeon/atom.c | 3 ++- > drivers/gpu/drm/v3d/v3d_debugfs.c | 11 ++++++----- > drivers/gpu/drm/virtio/virtgpu_debugfs.c | 3 ++- > .../net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 11 ----------- > include/linux/string_helpers.h | 4 ++++ > security/tomoyo/audit.c | 2 +- > security/tomoyo/common.c | 18 ++++-------------- > security/tomoyo/common.h | 1 - > 15 files changed, 31 insertions(+), 59 deletions(-)
On Wednesday, January 19, 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > Add some helpers under lib/string_helpers.h so they can be used > throughout the kernel. When I started doing this there were 2 other > previous attempts I know of, not counting the iterations each of them > had: > > 1) https://lore.kernel.org/all/20191023131308.9420-1-jani. > nikula@intel.com/ > 2) https://lore.kernel.org/all/20210215142137.64476-1-andriy. > shevchenko@linux.intel.com/#t > > Going through the comments I tried to find some common ground and > justification for what is in here, addressing some of the concerns > raised. > > a. This version should be a drop-in replacement for what is currently in > the tree, with no change in behavior or binary size. For binary > size what I checked wat that the linked objects in the end have the > same size (gcc 11). From comments in the previous attempts this seems > also the case for earlier compiler versions > > b. I didn't change the function name to choice_* as suggested by Andrew > Morton in 20191023155619.43e0013f0c8c673a5c508c1e@linux-foundation.org > because other people argumented in favor of shorter names for these > simple helpers - if they are long and people simply not use due to > that, we failed > > c. Use string_helper.h for these helpers - pulling string.h in the > compilations units was one of the concerns and I think re-using this > already existing header is better than creating a new string-choice.h > > d. This doesn't bring onoff() helper as there are some places in the > kernel with onoff as variable - another name is probably needed for > this function in order not to shadow the variable, or those variables > could be renamed. Or if people wanting <someprefix> > try to find a short one > > e. One alternative to all of this suggested by Christian König > (43456ba7-c372-84cc-4949-dcb817188e21@amd.com) would be to add a > printk format. But besides the comment, he also seemed to like > the common function. This brought the argument from others that the > simple yesno()/enabledisable() already used in the code is easier to > remember and use than e.g. %py[DOY] > > Last patch also has some additional conversion of open coded cases. I > preferred starting with drm/ since this is "closer to home". > > I hope this is a good summary of the previous attempts and a way we can > move forward. > > Andrew Morton, Petr Mladek, Andy Shevchenko: if this is accepted, my > proposal is to take first 2 patches either through mm tree or maybe > vsprintf. Last patch can be taken later through drm. I believe the best if we go via drm-misc with the entire series. I have couple of comments, after addressing them: Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > thanks > Lucas De Marchi > > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Ben Skeggs <bskeggs@redhat.com> > Cc: Christian König <christian.koenig@amd.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: David Airlie <airlied@linux.ie> > Cc: David S. Miller <davem@davemloft.net> > Cc: Emma Anholt <emma@anholt.net> > Cc: Eryk Brol <eryk.brol@amd.com> > Cc: Francis Laniel <laniel_francis@privacyrequired.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Harry Wentland <harry.wentland@amd.com> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Julia Lawall <julia.lawall@lip6.fr> > Cc: Kentaro Takeda <takedakn@nttdata.co.jp> > Cc: Leo Li <sunpeng.li@amd.com> > Cc: Mikita Lipski <mikita.lipski@amd.com> > Cc: Petr Mladek <pmladek@suse.com> > Cc: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com> > Cc: Raju Rangoju <rajur@chelsio.com> > Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Vishal Kulkarni <vishal@chelsio.com> > > Lucas De Marchi (3): > lib/string_helpers: Consolidate yesno() implementation > lib/string_helpers: Add helpers for enable[d]/disable[d] > drm: Convert open yes/no strings to yesno() > > drivers/gpu/drm/amd/amdgpu/atom.c | 3 ++- > .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 6 +----- > drivers/gpu/drm/drm_client_modeset.c | 3 ++- > drivers/gpu/drm/drm_dp_helper.c | 3 ++- > drivers/gpu/drm/drm_gem.c | 3 ++- > drivers/gpu/drm/i915/i915_utils.h | 15 --------------- > drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c | 4 +++- > drivers/gpu/drm/radeon/atom.c | 3 ++- > drivers/gpu/drm/v3d/v3d_debugfs.c | 11 ++++++----- > drivers/gpu/drm/virtio/virtgpu_debugfs.c | 3 ++- > .../net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 11 ----------- > include/linux/string_helpers.h | 4 ++++ > security/tomoyo/audit.c | 2 +- > security/tomoyo/common.c | 18 ++++-------------- > security/tomoyo/common.h | 1 - > 15 files changed, 31 insertions(+), 59 deletions(-) > > -- > 2.34.1 > >
On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote: > Add some helpers under lib/string_helpers.h so they can be used > throughout the kernel. When I started doing this there were 2 other > previous attempts I know of, not counting the iterations each of them > had: > > 1) https://lore.kernel.org/all/20191023131308.9420-1-jani.nikula@intel.com/ > 2) https://lore.kernel.org/all/20210215142137.64476-1-andriy.shevchenko@linux.intel.com/#t > > Going through the comments I tried to find some common ground and > justification for what is in here, addressing some of the concerns > raised. > > d. This doesn't bring onoff() helper as there are some places in the > kernel with onoff as variable - another name is probably needed for > this function in order not to shadow the variable, or those variables > could be renamed. Or if people wanting <someprefix> > try to find a short one I would call it str_on_off(). And I would actually suggest to use the same style also for the other helpers. The "str_" prefix would make it clear that it is something with string. There are other <prefix>_on_off() that affect some functionality, e.g. mute_led_on_off(), e1000_vlan_filter_on_off(). The dash '_' would significantly help to parse the name. yesno() and onoff() are nicely short and kind of acceptable. But "enabledisable()" is a puzzle. IMHO, str_yes_no(), str_on_off(), str_enable_disable() are a good compromise. The main motivation should be code readability. You write the code once. But many people will read it many times. Open coding is sometimes better than misleading macro names. That said, I do not want to block this patchset. If others like it... ;-) > e. One alternative to all of this suggested by Christian König > (43456ba7-c372-84cc-4949-dcb817188e21@amd.com) would be to add a > printk format. But besides the comment, he also seemed to like > the common function. This brought the argument from others that the > simple yesno()/enabledisable() already used in the code is easier to > remember and use than e.g. %py[DOY] Thanks for not going this way :-) > Last patch also has some additional conversion of open coded cases. I > preferred starting with drm/ since this is "closer to home". > > I hope this is a good summary of the previous attempts and a way we can > move forward. > > Andrew Morton, Petr Mladek, Andy Shevchenko: if this is accepted, my > proposal is to take first 2 patches either through mm tree or maybe > vsprintf. Last patch can be taken later through drm. I agree with Andy that it should go via drm tree. It would make it easier to handle potential conflicts. Just in case, you decide to go with str_yes_no() or something similar. Mass changes are typically done at the end on the merge window. The best solution is when it can be done by a script. Best Regards, Petr
On Wed, 19 Jan 2022, Petr Mladek <pmladek@suse.com> wrote: > On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote: >> Add some helpers under lib/string_helpers.h so they can be used >> throughout the kernel. When I started doing this there were 2 other >> previous attempts I know of, not counting the iterations each of them >> had: >> >> 1) https://lore.kernel.org/all/20191023131308.9420-1-jani.nikula@intel.com/ >> 2) https://lore.kernel.org/all/20210215142137.64476-1-andriy.shevchenko@linux.intel.com/#t >> >> Going through the comments I tried to find some common ground and >> justification for what is in here, addressing some of the concerns >> raised. >> >> d. This doesn't bring onoff() helper as there are some places in the >> kernel with onoff as variable - another name is probably needed for >> this function in order not to shadow the variable, or those variables >> could be renamed. Or if people wanting <someprefix> >> try to find a short one > > I would call it str_on_off(). > > And I would actually suggest to use the same style also for > the other helpers. > > The "str_" prefix would make it clear that it is something with > string. There are other <prefix>_on_off() that affect some > functionality, e.g. mute_led_on_off(), e1000_vlan_filter_on_off(). > > The dash '_' would significantly help to parse the name. yesno() and > onoff() are nicely short and kind of acceptable. But "enabledisable()" > is a puzzle. > > IMHO, str_yes_no(), str_on_off(), str_enable_disable() are a good > compromise. > > The main motivation should be code readability. You write the > code once. But many people will read it many times. Open coding > is sometimes better than misleading macro names. > > That said, I do not want to block this patchset. If others like > it... ;-) I don't mind the names either way. Adding the prefix and dashes is helpful in that it's possible to add the functions first and convert users at leisure, though with a bunch of churn, while using names that collide with existing ones requires the changes to happen in one go. What I do mind is grinding this series to a halt once again. I sent a handful of versions of this three years ago, with inconclusive bikeshedding back and forth, eventually threw my hands up in disgust, and walked away. > > >> e. One alternative to all of this suggested by Christian König >> (43456ba7-c372-84cc-4949-dcb817188e21@amd.com) would be to add a >> printk format. But besides the comment, he also seemed to like >> the common function. This brought the argument from others that the >> simple yesno()/enabledisable() already used in the code is easier to >> remember and use than e.g. %py[DOY] > > Thanks for not going this way :-) > >> Last patch also has some additional conversion of open coded cases. I >> preferred starting with drm/ since this is "closer to home". >> >> I hope this is a good summary of the previous attempts and a way we can >> move forward. >> >> Andrew Morton, Petr Mladek, Andy Shevchenko: if this is accepted, my >> proposal is to take first 2 patches either through mm tree or maybe >> vsprintf. Last patch can be taken later through drm. > > I agree with Andy that it should go via drm tree. It would make it > easier to handle potential conflicts. > > Just in case, you decide to go with str_yes_no() or something similar. > Mass changes are typically done at the end on the merge window. > The best solution is when it can be done by a script. > > Best Regards, > Petr
On Wed, Jan 19, 2022 at 04:16:12PM +0200, Jani Nikula wrote: > On Wed, 19 Jan 2022, Petr Mladek <pmladek@suse.com> wrote: > > On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote: > >> Add some helpers under lib/string_helpers.h so they can be used > >> throughout the kernel. When I started doing this there were 2 other > >> previous attempts I know of, not counting the iterations each of them > >> had: > >> > >> 1) https://lore.kernel.org/all/20191023131308.9420-1-jani.nikula@intel.com/ > >> 2) https://lore.kernel.org/all/20210215142137.64476-1-andriy.shevchenko@linux.intel.com/#t > >> > >> Going through the comments I tried to find some common ground and > >> justification for what is in here, addressing some of the concerns > >> raised. > >> > >> d. This doesn't bring onoff() helper as there are some places in the > >> kernel with onoff as variable - another name is probably needed for > >> this function in order not to shadow the variable, or those variables > >> could be renamed. Or if people wanting <someprefix> > >> try to find a short one > > > > I would call it str_on_off(). > > > > And I would actually suggest to use the same style also for > > the other helpers. > > > > The "str_" prefix would make it clear that it is something with > > string. There are other <prefix>_on_off() that affect some > > functionality, e.g. mute_led_on_off(), e1000_vlan_filter_on_off(). > > > > The dash '_' would significantly help to parse the name. yesno() and > > onoff() are nicely short and kind of acceptable. But "enabledisable()" > > is a puzzle. > > > > IMHO, str_yes_no(), str_on_off(), str_enable_disable() are a good > > compromise. > > > > The main motivation should be code readability. You write the > > code once. But many people will read it many times. Open coding > > is sometimes better than misleading macro names. > > > > That said, I do not want to block this patchset. If others like > > it... ;-) > > I don't mind the names either way. Adding the prefix and dashes is > helpful in that it's possible to add the functions first and convert > users at leisure, though with a bunch of churn, while using names that > collide with existing ones requires the changes to happen in one go. > > What I do mind is grinding this series to a halt once again. I sent a > handful of versions of this three years ago, with inconclusive > bikeshedding back and forth, eventually threw my hands up in disgust, > and walked away. Yeah we can sed this anytime later we want to, but we need to get the foot in the door. There's also a pile more of these all over. Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> on the series, maybe it helps? And yes let's merge this through drm-misc. -Daniel > > > > > > >> e. One alternative to all of this suggested by Christian König > >> (43456ba7-c372-84cc-4949-dcb817188e21@amd.com) would be to add a > >> printk format. But besides the comment, he also seemed to like > >> the common function. This brought the argument from others that the > >> simple yesno()/enabledisable() already used in the code is easier to > >> remember and use than e.g. %py[DOY] > > > > Thanks for not going this way :-) > > > >> Last patch also has some additional conversion of open coded cases. I > >> preferred starting with drm/ since this is "closer to home". > >> > >> I hope this is a good summary of the previous attempts and a way we can > >> move forward. > >> > >> Andrew Morton, Petr Mladek, Andy Shevchenko: if this is accepted, my > >> proposal is to take first 2 patches either through mm tree or maybe > >> vsprintf. Last patch can be taken later through drm. > > > > I agree with Andy that it should go via drm tree. It would make it > > easier to handle potential conflicts. > > > > Just in case, you decide to go with str_yes_no() or something similar. > > Mass changes are typically done at the end on the merge window. > > The best solution is when it can be done by a script. > > > > Best Regards, > > Petr > > -- > Jani Nikula, Intel Open Source Graphics Center
On Wed, Jan 19, 2022 at 05:15:02PM +0100, Daniel Vetter wrote: >On Wed, Jan 19, 2022 at 04:16:12PM +0200, Jani Nikula wrote: >> On Wed, 19 Jan 2022, Petr Mladek <pmladek@suse.com> wrote: >> > On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote: >> >> Add some helpers under lib/string_helpers.h so they can be used >> >> throughout the kernel. When I started doing this there were 2 other >> >> previous attempts I know of, not counting the iterations each of them >> >> had: >> >> >> >> 1) https://lore.kernel.org/all/20191023131308.9420-1-jani.nikula@intel.com/ >> >> 2) https://lore.kernel.org/all/20210215142137.64476-1-andriy.shevchenko@linux.intel.com/#t >> >> >> >> Going through the comments I tried to find some common ground and >> >> justification for what is in here, addressing some of the concerns >> >> raised. >> >> >> >> d. This doesn't bring onoff() helper as there are some places in the >> >> kernel with onoff as variable - another name is probably needed for >> >> this function in order not to shadow the variable, or those variables >> >> could be renamed. Or if people wanting <someprefix> >> >> try to find a short one >> > >> > I would call it str_on_off(). >> > >> > And I would actually suggest to use the same style also for >> > the other helpers. >> > >> > The "str_" prefix would make it clear that it is something with >> > string. There are other <prefix>_on_off() that affect some >> > functionality, e.g. mute_led_on_off(), e1000_vlan_filter_on_off(). >> > >> > The dash '_' would significantly help to parse the name. yesno() and >> > onoff() are nicely short and kind of acceptable. But "enabledisable()" >> > is a puzzle. >> > >> > IMHO, str_yes_no(), str_on_off(), str_enable_disable() are a good >> > compromise. >> > >> > The main motivation should be code readability. You write the >> > code once. But many people will read it many times. Open coding >> > is sometimes better than misleading macro names. >> > >> > That said, I do not want to block this patchset. If others like >> > it... ;-) >> >> I don't mind the names either way. Adding the prefix and dashes is >> helpful in that it's possible to add the functions first and convert >> users at leisure, though with a bunch of churn, while using names that >> collide with existing ones requires the changes to happen in one go. >> >> What I do mind is grinding this series to a halt once again. I sent a >> handful of versions of this three years ago, with inconclusive >> bikeshedding back and forth, eventually threw my hands up in disgust, >> and walked away. > >Yeah we can sed this anytime later we want to, but we need to get the foot >in the door. There's also a pile more of these all over. > >Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >on the series, maybe it helps? And yes let's merge this through drm-misc. Ok, it seems we are reaching some agreement here then: - Change it to use str_ prefix - Wait -rc1 to avoid conflict - Merge through drm-misc I will re-send the series again soon. Lucas De Marchi
On Wed, Jan 19, 2022 at 12:53:43PM -0800, Lucas De Marchi wrote: > On Wed, Jan 19, 2022 at 05:15:02PM +0100, Daniel Vetter wrote: > > On Wed, Jan 19, 2022 at 04:16:12PM +0200, Jani Nikula wrote: ... > > Yeah we can sed this anytime later we want to, but we need to get the foot > > in the door. There's also a pile more of these all over. > > > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > on the series, maybe it helps? And yes let's merge this through drm-misc. > > Ok, it seems we are reaching some agreement here then: > - Change it to use str_ prefix Not sure about this (*), but have no strong argument against. Up to you. Ah, yes, Jani said about additional churn this change will make if goes together with this series. Perhaps it can be done separately? > - Wait -rc1 to avoid conflict > - Merge through drm-misc *) E.g. yesno() to me doesn't sound too bad to misunderstand its meaning, esp.when it's used as an argument to the printf() functions.
On Wed 2022-01-19 16:16:12, Jani Nikula wrote: > On Wed, 19 Jan 2022, Petr Mladek <pmladek@suse.com> wrote: > > On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote: > >> d. This doesn't bring onoff() helper as there are some places in the > >> kernel with onoff as variable - another name is probably needed for > >> this function in order not to shadow the variable, or those variables > >> could be renamed. Or if people wanting <someprefix> > >> try to find a short one > > > > I would call it str_on_off(). > > > > And I would actually suggest to use the same style also for > > the other helpers. > > > > The "str_" prefix would make it clear that it is something with > > string. There are other <prefix>_on_off() that affect some > > functionality, e.g. mute_led_on_off(), e1000_vlan_filter_on_off(). > > > > The dash '_' would significantly help to parse the name. yesno() and > > onoff() are nicely short and kind of acceptable. But "enabledisable()" > > is a puzzle. > > > > IMHO, str_yes_no(), str_on_off(), str_enable_disable() are a good > > compromise. > > > > The main motivation should be code readability. You write the > > code once. But many people will read it many times. Open coding > > is sometimes better than misleading macro names. > > > > That said, I do not want to block this patchset. If others like > > it... ;-) > > I don't mind the names either way. Adding the prefix and dashes is > helpful in that it's possible to add the functions first and convert > users at leisure, though with a bunch of churn, while using names that > collide with existing ones requires the changes to happen in one go. It is also possible to support both notations at the beginning. And convert the existing users in the 2nd step. > What I do mind is grinding this series to a halt once again. I sent a > handful of versions of this three years ago, with inconclusive > bikeshedding back and forth, eventually threw my hands up in disgust, > and walked away. Yeah, and I am sorry for bikeshedding. Honestly, I do not know what is better. This is why I do not want to block this series when others like this. My main motivation is to point out that: enabledisable(enable) might be, for some people, more eye bleeding than enable ? "enable" : "disable" The problem is not that visible with yesno() and onoff(). But as you said, onoff() confliscts with variable names. And enabledisable() sucks. As a result, there is a non-trivial risk of two mass changes: now: - contition ? "yes" : "no" + yesno(condition) a few moths later: - yesno(condition) + str_yes_no(condition) Best Regards, Petr
... > Yeah, and I am sorry for bikeshedding. Honestly, I do not know what is > better. This is why I do not want to block this series when others > like this. > > My main motivation is to point out that: > > enabledisable(enable) > > might be, for some people, more eye bleeding than > > enable ? "enable" : "disable" Indeed - you need to look the former up, wasting brain time. > The problem is not that visible with yesno() and onoff(). But as you said, > onoff() confliscts with variable names. And enabledisable() sucks. > As a result, there is a non-trivial risk of two mass changes: > > now: > > - contition ? "yes" : "no" > + yesno(condition) > > a few moths later: > > - yesno(condition) > + str_yes_no(condition) Followed by: - str_yes_no(x) + no_yes_str(x) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Thu, 20 Jan 2022, Petr Mladek <pmladek@suse.com> wrote: > On Wed 2022-01-19 16:16:12, Jani Nikula wrote: >> On Wed, 19 Jan 2022, Petr Mladek <pmladek@suse.com> wrote: >> > On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote: >> >> d. This doesn't bring onoff() helper as there are some places in the >> >> kernel with onoff as variable - another name is probably needed for >> >> this function in order not to shadow the variable, or those variables >> >> could be renamed. Or if people wanting <someprefix> >> >> try to find a short one >> > >> > I would call it str_on_off(). >> > >> > And I would actually suggest to use the same style also for >> > the other helpers. >> > >> > The "str_" prefix would make it clear that it is something with >> > string. There are other <prefix>_on_off() that affect some >> > functionality, e.g. mute_led_on_off(), e1000_vlan_filter_on_off(). >> > >> > The dash '_' would significantly help to parse the name. yesno() and >> > onoff() are nicely short and kind of acceptable. But "enabledisable()" >> > is a puzzle. >> > >> > IMHO, str_yes_no(), str_on_off(), str_enable_disable() are a good >> > compromise. >> > >> > The main motivation should be code readability. You write the >> > code once. But many people will read it many times. Open coding >> > is sometimes better than misleading macro names. >> > >> > That said, I do not want to block this patchset. If others like >> > it... ;-) >> >> I don't mind the names either way. Adding the prefix and dashes is >> helpful in that it's possible to add the functions first and convert >> users at leisure, though with a bunch of churn, while using names that >> collide with existing ones requires the changes to happen in one go. > > It is also possible to support both notations at the beginning. > And convert the existing users in the 2nd step. > >> What I do mind is grinding this series to a halt once again. I sent a >> handful of versions of this three years ago, with inconclusive >> bikeshedding back and forth, eventually threw my hands up in disgust, >> and walked away. > > Yeah, and I am sorry for bikeshedding. Honestly, I do not know what is > better. This is why I do not want to block this series when others > like this. > > My main motivation is to point out that: > > enabledisable(enable) > > might be, for some people, more eye bleeding than > > enable ? "enable" : "disable" > > > The problem is not that visible with yesno() and onoff(). But as you said, > onoff() confliscts with variable names. And enabledisable() sucks. > As a result, there is a non-trivial risk of two mass changes: My point is, in the past three years we could have churned through more than two mass renames just fine, if needed, *if* we had just managed to merge something for a start! BR, Jani. > > now: > > - contition ? "yes" : "no" > + yesno(condition) > > a few moths later: > > - yesno(condition) > + str_yes_no(condition) > > > Best Regards, > Petr
On Thu 2022-01-20 11:12:27, Jani Nikula wrote: > On Thu, 20 Jan 2022, Petr Mladek <pmladek@suse.com> wrote: > > The problem is not that visible with yesno() and onoff(). But as you said, > > onoff() confliscts with variable names. And enabledisable() sucks. > > As a result, there is a non-trivial risk of two mass changes: > > My point is, in the past three years we could have churned through more > than two mass renames just fine, if needed, *if* we had just managed to > merge something for a start! Huh, this sound alarming. Cosmetic changes just complicate history. They make "git blame" useless. They also complicate backports. I know that it is not problem for mainline. But there are supported stable branches, ... There should be a good reason for such changes. They should not be done light-heartedly. Best Regards, Petr