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: <20241121115230.u6s3frtwg25afdbg@skbuf>
Date: Thu, 21 Nov 2024 13:52:30 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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 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.