On 15.08.24 15:11, Andreas Hindborg wrote: > "Benno Lossin" <benno.lossin@xxxxxxxxx> writes: >> On 05.08.24 12:55, Andreas Hindborg wrote: >>> "Benno Lossin" <benno.lossin@xxxxxxxxx> writes: >>>> On 02.08.24 12:27, Andreas Hindborg wrote: >>>>> At a higher level where the bindings supply the parsing functions, we >>>>> can decide that passing an argument without a value yields a default >>>>> parameter value. C does this for the predefined `bool` type. The >>>>> predefined integer types does not support omitting the value. >>>>> >>>>> This patch only supports the higher level predefined parameter types, >>>>> and does not allow modules to supply their own parameter parsing >>>>> functions. None of the types we implement in this patch support passing >>>>> the argument without a value. This is intentional to mirror the C >>>>> implementation. >>>>> >>>>> To that end, I removed `NOARG_ALLOWED`, and changed the parsing function >>>>> trait to: >>>>> >>>>> fn try_from_param_arg(arg: &'static [u8]) -> Result<Self>; >>>>> >>>>> If/when we start supporting types like `bool` or custom parsing >>>>> functions provided by the module, we will have to update the signature >>>>> to take an `Option` to represent the case where the user passed an >>>>> argument without a value. However, to mimic C, the function must always >>>>> return a value if successful, even if the user did not supply a value to >>>>> the argument. >>>>> >>>>> Two different default values are in flight here. 1) the value that the >>>>> parameter will have before the kernel calls `try_from_param_arg` via >>>>> `set_param` and 2) the value to return from `try_from_param_arg` if the >>>>> user did not pass a value with the argument. >>>>> >>>>> For a `bool` 1) would usually be `false` and 2) would always be `true`. >>>>> >>>>> For predefined types the module would not customize 2), but 1) is useful >>>>> to customize. For custom types where the module supplies the parsing >>>>> function, 2) would be implicitly given by the module in the parsing >>>>> function. >>>>> >>>>> In this patch set, we only have 1 default value, namely 1). We do not >>>>> need 2) because we do not support parameters without values. >>>> >>>> I am not sure that putting the default value of `my_module.param` into >>>> the `ModuleParam` trait is a good idea. It feels more correct to me to >>>> add an optional field to the part in `module!` that can be set to denote >>>> this default value -- we might also want to change the name of >>>> `default`, what do you think of `default_inactive` and `default_active`? >>> >>> For all the predefined parameter types, the module code would never set >>> the `default_active` value. It should be part of the data parsing >>> specification for the predefined argument types. >> >> So if your module has an i32 parameter, you can't set a default value to >> eg 1000? > > You _would_ be able to set the `default_inactive` value, which is the > value assigned to the static at initialization time. It would make sense > to default this to 0 for integer types and make it overridable in the > `module!` macro. Hmm, I would say it makes more sense to have the user always specify a default in `module!`, since integers can mean a lot of different things. > You would not be to set the `default_active` value, which is the value > assigned to the parameter static variable, when the parameter is passed > without value. The reason being that we want to mirror C, so we prohibit > this for predefined integer parameter types. Gotcha, I wasn't 100% sure in our previous emails if we wanted to exactly mirror C or not. Should've asked that. Thanks for taking the time to write this clarification (and the other ones as well!), I think I now finally understand why you do it this way. I agree with your approach. --- Cheers, Benno
