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

[Vladimir Oltean <olteanv@gmail.com>]

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.