On Tue, Sep 03, 2024 at 02:25:53PM +0800, Hongbo Li wrote:

On 2024/9/2 22:30, Willem de Bruijn wrote:

quoted

Andy Shevchenko wrote:

quoted

On Sat, Aug 31, 2024 at 01:07:41PM -0700, Jakub Kicinski wrote:

quoted

On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote:

...

quoted

quoted

quoted

quoted

  		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.

quoted

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