netdev - Re: [PATCH] net: phylink: Separating two unrelated definitions for improving code readability

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zz8jVmO82CHQe5jR@shell.armlinux.org.uk>
Date: Thu, 21 Nov 2024 12:11:02 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Cong Yi <yicong.srfy@...mail.com>, andrew@...n.ch, hkallweit1@...il.com,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	yicong@...inos.cn
Subject: Re: [PATCH] net: phylink: Separating two unrelated definitions for
 improving code readability

On Thu, Nov 21, 2024 at 01:52:30PM +0200, Vladimir Oltean wrote:
> On Thu, Nov 21, 2024 at 11:21:33AM +0000, Russell King (Oracle) wrote:
> > On Thu, Nov 21, 2024 at 12:50:44PM +0200, Vladimir Oltean wrote:
> > > On Wed, Nov 20, 2024 at 05:46:14PM +0800, Cong Yi wrote:
> > > > Hi, Russell King:
> > > > 
> > > > Thank you for your reply!
> > > > Yes, as you say, there is no problem with the definitions themselves
> > > > being named. When I just read from Linux-5.4 to 6.6, I thought
> > > > that PCS_STATE_ and PHYLINK_DISABLE- were associated in some way.
> > > > After reading the code carefully, I found that there was no correlation。
> > > > In order to avoid similar confusion, I sent this patch.
> > > 
> > > For the record, I agree that tying together unrelated constants inside
> > > the same anonymous enum and resetting the counter is a confusing coding
> > > pattern, to which I don't see the benefit. Separating them and giving
> > > names to the enums also gives the opportunity for stronger typing, which
> > > was done here. I think the patch (or at least its idea) is ok.
> > 
> > See include/linux/ata.h, and include/linux/libata.h.
> > 
> > We also have many enums that either don't use the enum counter, or set
> > the counter to a specific value.
> > 
> > The typing argument is nonsense. This is a common misconception by C
> > programmers. You don't get any extra typechecking with enums. If you
> > define two enums, say fruit and colour, this produces no warning,
> > even with -Wall -pedantic:
> > 
> > enum fruit { APPLE, ORANGE };
> > enum colour { BLACK, WHITE };
> > enum fruit get_fruit(void);
> > enum colour test(void)
> > {
> > 	return get_fruit();
> > }
> > 
> > What one gets is more compiler specific variability in the type -
> > some compiler architectures may use storage sufficient to store the
> > range of values defined in the enum (e.g. it may select char vs int
> > vs long) which makes laying out structs with no holes harder.
> 
> Well, I mean...
> 
> $ cat test_enum.c
> #include <stdio.h>
> 
> enum fruit { APPLE, ORANGE };
> enum colour { BLACK, WHITE };
> 
> enum fruit get_fruit(void)
> {
> 	return APPLE;
> }
> 
> enum colour test(void)
> {
> 	return get_fruit();
> }
> 
> int main(void)
> {
> 	test();
> }
> $ make CFLAGS="-Wall -Wextra" test_enum
> cc -Wall -Wextra    test_enum.c   -o test_enum
> test_enum.c: In function ‘test’:
> test_enum.c:13:16: warning: implicit conversion from ‘enum fruit’ to ‘enum colour’ [-Wenum-conversion]
>    13 |         return get_fruit();
>       |                ^~~~~~~~~~~
> 
> I don't understand what's to defend about this, really.

It's not something I want to entertain right now. I have enough on my
plate without having patches like this to deal with. Maybe next year
I'll look at it, but not right now.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!