On 01/06/2023 17:22, Glen Choo wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> 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.
That's great
- (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.
I agree that keeping the churn to the function signatures makes it
bearable. I wonder though if we could make the change by doing
-git_default_config(const char *key, const char *value, void *data)
+git_default_config(const struct key_value_info *kvi, void *data)
{
+ const char *key = kvi_key(kvi);
+ const char *value = kvi_value(kvi);
+
That would add to the diffstat but I think it wouldn't really be any
harder to review than just changing the signature as we're not modifying
any existing lines in the function body, just adding a couple of
variable declarations to the start of the function. If there is an error
in either of the variable declarations then the compiler will complain
as "key" or "value" will end up not being declared. It would pave the
way for gradually changing the function bodies to use "kvi" directly and
removing "key" and "value"
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.
That's an interesting possibility, I worry though that two huge changes
to the config callbacks might be one too many though.
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.
It could be done gradually though, converting one config callback at a
time once the relevant changes have been made to config.c.
As an aside, I think we'd also want a couple of helpers for matching
keys so we can just write kvi_match_key(kvi, "user.name") or
kvi_skip_key_prefix(kvi, "core.", &p) rather than having to extract the
key name first.
Best Wishes
Phillip
