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. -- With Best Regards, Andy Shevchenko
