Re: [PATCH v2 03/14] (RFC-only) config: add kvi arg to config_fn_t

[Glen Choo <chooglen@google.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

>   - is it worth making struct key_value_info opaque and provide getters
>     for the fields so we can change the implementation in the future
>     without having to modify every user. We could rename it
>     config_context or something generic like that if we think it might
>     grow in scope in the future.

Yes! I planned to do the key_value_info -> config_context conversion
when I send the first non-RFC version for this exact reason.

>   - (probably impractical) could we stuff the key and value into struct
>     key_value_info so config_fn_t becomes
>     fn(const struct key_value_info, void *data)
>     that would get rid of all the UNUSED annotations but would mean even
>     more churn.

Some of my colleagues also suggested this off-list. I think it is
impractical for this series because I don't think anyone could
reasonably review with all of the added churn. At least its current
form, the churn is mostly concentrated in the signatures, but performing
^this change would make the bodies quite churny too.

After this series, I think it becomes somewhat feasible with coccinelle.
My .cocci files were difficult to craft because we couldn't rely on the
signature of config_fn_t alone to tell us if the function is actually
used as config_fn_t, but after this series, we can just use the
signature since config_fn_t has a struct key_value_info param.

>     The advantage is that one could add functions like
>     kvi_bool_or_int(kvi, &is_bool) and get good error messages because
>     all the config parsing functions would all have access to location
>     information.

Interesting, I hadn't considered this possibility. This seems like a
pretty good abstraction to me, though I worry about the feasibility
since this is yet again more churn.