| Message ID | 20240831095840.4173362-3-lihongbo22@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce several opposite string choice helpers | expand |
On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote: > Use str_disabled_enabled() helper instead of open > coding the same. > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 6fe5e8f7017c..29647704bda8 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -3178,7 +3178,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, > > /* [unimplemented] */ > netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n", > - arg ? "disabled" : "enabled"); > + str_disabled_enabled(arg)); You don't explain the 'why'. How is this an improvement? nack on this and 2 similar networking changes you sent
On 2024/9/1 4:07, Jakub Kicinski wrote: > On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote: >> Use str_disabled_enabled() helper instead of open >> coding the same. > >> diff --git a/drivers/net/tun.c b/drivers/net/tun.c >> index 6fe5e8f7017c..29647704bda8 100644 >> --- a/drivers/net/tun.c >> +++ b/drivers/net/tun.c >> @@ -3178,7 +3178,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, >> >> /* [unimplemented] */ >> netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n", >> - arg ? "disabled" : "enabled"); >> + str_disabled_enabled(arg)); > > You don't explain the 'why'. How is this an improvement? > nack on this and 2 similar networking changes you sent > This just give an example of using lib/string_choices' helper which prevents the function from being removed due to detection of non-use. These helpers are convenient. It's functionally equivalent, this avoids the dispersion of such writing styles( ? XXX : noXXX) everywhere. Thanks, Hongbo
On 31/08/2024 23:07, Jakub Kicinski wrote: > On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote: >> Use str_disabled_enabled() helper instead of open >> coding the same. > >> diff --git a/drivers/net/tun.c b/drivers/net/tun.c >> index 6fe5e8f7017c..29647704bda8 100644 >> --- a/drivers/net/tun.c >> +++ b/drivers/net/tun.c >> @@ -3178,7 +3178,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, >> >> /* [unimplemented] */ >> netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n", >> - arg ? "disabled" : "enabled"); >> + str_disabled_enabled(arg)); > > You don't explain the 'why'. How is this an improvement? > nack on this and 2 similar networking changes you sent > Are you against the concept of string_choices in general, or this specific change?
On Sat, Aug 31, 2024 at 01:07:41PM -0700, Jakub Kicinski wrote: > On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote: > > Use str_disabled_enabled() helper instead of open > > coding the same. ... > > netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n", > > - arg ? "disabled" : "enabled"); > > + str_disabled_enabled(arg)); > > You don't explain the 'why'. How is this an improvement? > nack on this and 2 similar networking changes you sent Side opinion: This makes the messages more unified and not prone to typos and/or grammatical mistakes. Unification allows to shrink binary due to linker efforts on string literals deduplication. That said, I see an improvement here, however it might be not recognised as a Big Win. And yes, I agree on the commit message poor explanations.
Andy Shevchenko wrote: > On Sat, Aug 31, 2024 at 01:07:41PM -0700, Jakub Kicinski wrote: > > On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote: > > > Use str_disabled_enabled() helper instead of open > > > coding the same. > > ... > > > > netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n", > > > - arg ? "disabled" : "enabled"); > > > + str_disabled_enabled(arg)); > > > > You don't explain the 'why'. How is this an improvement? > > nack on this and 2 similar networking changes you sent > > Side opinion: This makes the messages more unified and not prone to typos > and/or grammatical mistakes. Unification allows to shrink binary due to > linker efforts on string literals deduplication. This adds a layer of indirection. The original code is immediately obvious. When I see the new code I have to take a detour through cscope to figure out what it does. To me, in this case, the benefit is too marginal to justify that.
On Mon, 2 Sep 2024 09:10:33 +0300 Gal Pressman wrote: > > You don't explain the 'why'. How is this an improvement? > > nack on this and 2 similar networking changes you sent > > Are you against the concept of string_choices in general, or this > specific change? Willem verbalized my opinion better than I can. If a (driver|subsystem) maintainer likes string_choices that's perfectly fine, but IMHO they are not worth converting to, and not something we should suggest during review.
On 2024/9/2 22:30, Willem de Bruijn wrote: > Andy Shevchenko wrote: >> On Sat, Aug 31, 2024 at 01:07:41PM -0700, Jakub Kicinski wrote: >>> On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote: >>>> Use str_disabled_enabled() helper instead of open >>>> coding the same. >> >> ... >> >>>> netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n", >>>> - arg ? "disabled" : "enabled"); >>>> + str_disabled_enabled(arg)); >>> >>> You don't explain the 'why'. How is this an improvement? >>> nack on this and 2 similar networking changes you sent >> >> Side opinion: This makes the messages more unified and not prone to typos >> and/or grammatical mistakes. Unification allows to shrink binary due to >> linker efforts on string literals deduplication. > > This adds a layer of indirection. > > The original code is immediately obvious. When I see the new code I > have to take a detour through cscope to figure out what it does. If they have used it once, there is no need for more jumps, because it's relatively simple. Using a dedicated function seems very elegant and unified, especially for some string printing situations, such as disable/enable. Even in today's kernel tree, there are several different formats that appear: 'enable/disable', 'enabled/disabled', 'en/dis'. Thanks, Hongbo > > To me, in this case, the benefit is too marginal to justify that.
On Tue, Sep 03, 2024 at 02:25:53PM +0800, Hongbo Li wrote: > On 2024/9/2 22:30, Willem de Bruijn wrote: > > Andy Shevchenko wrote: > > > On Sat, Aug 31, 2024 at 01:07:41PM -0700, Jakub Kicinski wrote: > > > > On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote: ... > > > > > netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n", > > > > > - arg ? "disabled" : "enabled"); > > > > > + str_disabled_enabled(arg)); > > > > > > > > You don't explain the 'why'. How is this an improvement? > > > > nack on this and 2 similar networking changes you sent > > > > > > Side opinion: This makes the messages more unified and not prone to typos > > > and/or grammatical mistakes. Unification allows to shrink binary due to > > > linker efforts on string literals deduplication. > > > > This adds a layer of indirection. > > > > The original code is immediately obvious. When I see the new code I > > have to take a detour through cscope to figure out what it does. > If they have used it once, there is no need for more jumps, because it's > relatively simple. > > Using a dedicated function seems very elegant and unified, especially for > some string printing situations, such as disable/enable. Even in today's > kernel tree, there are several different formats that appear: > 'enable/disable', 'enabled/disabled', 'en/dis'. Not to mention that the longer word is the more error prone the spelling. > > To me, in this case, the benefit is too marginal to justify that. Hongbo, perhaps you need to add a top comment to the string_choices.h to explain the following: 1) the convention to use is str_$TRUE_$FALSE(), where $TRUE and $FALSE the respective words printed; 2) the pros of having unified output, 3) including but not limited to the linker deduplication facilities, making the binary smaller. With that you may always point people to the ad-hoc documentation.
On 2024/9/3 23:09, Andy Shevchenko wrote: > On Tue, Sep 03, 2024 at 02:25:53PM +0800, Hongbo Li wrote: >> On 2024/9/2 22:30, Willem de Bruijn wrote: >>> Andy Shevchenko wrote: >>>> On Sat, Aug 31, 2024 at 01:07:41PM -0700, Jakub Kicinski wrote: >>>>> On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote: > > ... > >>>>>> netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n", >>>>>> - arg ? "disabled" : "enabled"); >>>>>> + str_disabled_enabled(arg)); >>>>> >>>>> You don't explain the 'why'. How is this an improvement? >>>>> nack on this and 2 similar networking changes you sent >>>> >>>> Side opinion: This makes the messages more unified and not prone to typos >>>> and/or grammatical mistakes. Unification allows to shrink binary due to >>>> linker efforts on string literals deduplication. >>> >>> This adds a layer of indirection. >>> >>> The original code is immediately obvious. When I see the new code I >>> have to take a detour through cscope to figure out what it does. >> If they have used it once, there is no need for more jumps, because it's >> relatively simple. >> >> Using a dedicated function seems very elegant and unified, especially for >> some string printing situations, such as disable/enable. Even in today's >> kernel tree, there are several different formats that appear: >> 'enable/disable', 'enabled/disabled', 'en/dis'. > > Not to mention that the longer word is the more error prone the spelling. > >>> To me, in this case, the benefit is too marginal to justify that. > > Hongbo, perhaps you need to add a top comment to the string_choices.h to > explain the following: > 1) the convention to use is str_$TRUE_$FALSE(), where $TRUE and $FALSE the > respective words printed; > 2) the pros of having unified output, > 3) including but not limited to the linker deduplication facilities, making > the binary smaller. > > With that you may always point people to the ad-hoc documentation. > ok, thank you, and I can try it. However, with these modifications, I'm not sure whether Willem and Jakub agree with the changes. If they don't agree, then I'll have to remove this example in the next version. In the future, we can guide other developers to use these helpers directly instead of rewriting it themselves. Thanks, Hongbo
On Wed, 4 Sep 2024 10:27:18 +0800 Hongbo Li wrote: > However, with these modifications, I'm not sure whether Willem and Jakub > agree with the changes. If they don't agree, then I'll have to remove > this example in the next version. This and, to be clear, patch 4 as well. > In the future, we can guide other > developers to use these helpers directly instead of rewriting it themselves.
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 6fe5e8f7017c..29647704bda8 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -3178,7 +3178,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, /* [unimplemented] */ netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n", - arg ? "disabled" : "enabled"); + str_disabled_enabled(arg)); break; case TUNSETPERSIST:
Use str_disabled_enabled() helper instead of open coding the same. Signed-off-by: Hongbo Li <lihongbo22@huawei.com> --- drivers/net/tun.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)