Re: [patch 5/6] IP support for PPP

[Marcel Holtmann <marcel@holtmann.org>]

[-- Attachment #1: Type: text/plain, Size: 1608 bytes --]

Hi Kristen,

> > > +/****** IPCP support ****************/
> > > +enum {
> > > +	/* supported codes */
> > > +	IPCP_SUPPORTED_CODES	= (1 << CONFIGURE_REQUEST) |
> > > +				  (1 << CONFIGURE_ACK) |
> > > +				  (1 << CONFIGURE_NAK) |
> > > +				  (1 << CONFIGURE_REJECT) |
> > > +				  (1 << TERMINATE_REQUEST) |
> > > +				  (1 << TERMINATE_ACK) |
> > > +				  (1 << CODE_REJECT),
> > > +
> > > +	IPCP_PROTO		= 0x8021,
> > > +
> > > +	/* option types */
> > > +	IP_ADDRESSES		= 1,
> > > +	IP_COMPRESSION_PROTO	= 2,
> > > +	IP_ADDRESS		= 3,
> > > +	PRIMARY_DNS_SERVER	= 129,
> > > +	SECONDARY_DNS_SERVER	= 131,
> > > +};
> > 
> > I don't think enum is the right way for this. I can see it for the
> > option types, but for IPCP_PROTO and IPCP_SUPPORTED_CODES. I would say
> > just using simple defines is way better.
> > 
> > Feel free to convince me other way or show me what I have missed here.
> 
> It's mostly just a habit, but in general there are advantages to
> using enum vs. a define.  You are assured that the scope is kept local,
> even if you are uncreative with your name choices, whereas with a macro 
> it is not.  Also it's sometimes easier to debug with an enum vs. a macro.  
> I can certainly change it if you really want me too.

I have nothing against enums. I actually like them a lot if used
correctly, because gcc and gdb does make your life easier.

The thing that I dislike is clashing namespaces and scopes in just one
enum. In the example of both you mix two or more things that should at
least in different enums.

Regards

Marcel