Implementing new options could be simplified

[HayleyDeckers]

Currently, new DHCP Options need to be added in 7 different places:

1. Added to the OptionCode enum
2. From<u8> for OptionCode
3. From<OptionCode> for u8
4. Added to the DhcpOption enum
5. decode_inner
6. and encode
7. From<&DhcpOption> for OptionCode

Aside from the extra typing, it increases the risk of human error and adds visual clutter. Would you consider a PR that replaces these 7 steps (or a subsection of them) with a proc-macro?

I have a small prototype that looks like

/// implements step 1 to 4 given sets of
/// {numeric_code, TypeName, "extra doc comment", (DataType)[optional] }
macros::declare_codes!(
    {0, Pad, "Padding"},
    {1,  SubnetMask, "Subnet Mask", (Ipv4Addr)},
    {2,  TimeOffset, "Time Offset", (i32)},
    {3,  Router, "Router", (Vec<Ipv4Addr>)},
    ///...
    {255, End, "end-of-list marker"}
);

That I'd be willing to PR after some small tweaks, if desired :)

[leshow]

I've thought about this for a little bit and indeed the implementation of new options is a little boilerplate-y. I'm not against a proc-macro.

A few months ago I modified the num_enum crate and used that to derive the From<u8> for T & From<T> for uX impls (so 1-3 in your list). One thing I liked about this was that it was broadly useful everywhere we want to turn enums into numbers, not just for DhcpOption. What do you think of that? If we could modify that to also cover case 7 somehow. Either with a different macro or by further modifying num_enum, that would seem to me to cover the important bits and still leave the implementation of encode/decode flexible.

forgot to paste the link: https://github.com/bluecatengineering/dhcproto/blob/num_enum_experiment/src/v4/options.rs#L211

[HayleyDeckers]

A few months ago I modified the num_enum crate and used that to derive the From for T & From for uX impls (so 1-3 in your list). One thing I liked about this was that it was broadly useful everywhere we want to turn enums into numbers, not just for DhcpOption. What do you think of that?

Splitting of the number-to-enum and vice-versa into a common macro is something I've thought about too. Not the first time I've had this exact pattern of known-values + catch_all (or invalid) but there's a few ways to implement that . Could be a separate crate used by the declare_options macro too.

If we could modify that to also cover case 7 somehow. Either with a different macro or by further modifying num_enum, that would seem to me to cover the important bits

Using a derive-macro like num_enum would still require you to duplicate definitions between OptionCode and DhcpOption, so I'd suggest keeping a single declaration macro to generate both.

and still leave the implementation of encode/decode flexible.

I haven't looked into those bits yet. First thought is I would keep the decoding implementations outside of the macro, and define a generic implementation for e.g. u32::decode(_) -> Result<Self>, <Vec<Ipv4Addr> as Decoder>::decode(_) -> Result<Self> or similar. The macro could then group all DhcpOption variants by their payload type, and deffer to those functions.

[leshow]

My inclination is to leverage the macro to just generate the type and the From impls (1-4 & 7), and leave the decode/encode impls outside the macro, but we can discuss in the PR if you had something else. Happy to take the PR for the options macro if you want to submit that!