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

From: Glen Choo <chooglen@google.com>
Date: Jun 1, 2023, 16:22 UTC
Message-ID: <kl6l4jnr17am.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <6faf1b17-a1ca-0c22-2e43-aee121c4e36a@gmail.com>

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

Show 5 quoted lines

>   - 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.

Show 5 quoted lines

>   - (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.