Re: [PATCH v5 2/3] advice: revamp advise API
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Feb 26, 2020, 21:48 UTC
- Message-ID
- <20200226214811.227720-1-jonathantanmy@google.com>
- In-Reply-To
- <xmqq8skqkq35.fsf@gitster-ct.c.googlers.com>
Show 29 quoted lines
> Heba Waly <heba.waly@gmail.com> writes:
>
> > I'm not against this approach as well, but as I mentioned above, we
> > need a list of keys to be returned by list_config_advices(), that's
> > why defining the constant strings will not be sufficient in our case.
>
> Sorry, but I do not get it.
>
> Either you use enum or a bunch of variables of type const char [],
> "list all of them" would need an array whose elements are all of
> them, so
>
> const char ADVICE_FOO[] = "advice.foo";
> const char ADVICE_BAR[] = "advice.bar";
> ...
>
> static const char *all_advice_type[] = {
> ADVICE_FOO, ADVICE_BAR, ...
> };
>
> void for_each_advice_type(int (*fn)(const char *name))
> {
> int i;
> for (i = 0; i < ARRAY_SIZE(all_advice_type); i++)
> fn(all_advice_type[i]);
> }
>
> would be sufficient, and I do not think it takes any more effort to
> create and manage than using an array indexed with the enum, no?With the enum:
(.h)
enum advice_type {
ADVICE_FOO,
ADVICE_BAR
};(.c)
static const char *advice_config_keys[] = {
[ADVICE_FOO] = "advice.foo",
[ADVICE_BAR] = "advice.bar"
};
/* No need for all_advice_type because we can loop over advice_config_keys */With the bunch of variables of type const char []:
(.h) extern const char ADVICE_FOO[]; extern const char ADVICE_BAR[];
(.c)
const char ADVICE_FOO[] = "advice.foo";
const char ADVICE_BAR[] = "advice.bar";
static const char *all_advice_type[] = {
ADVICE_FOO,
ADVICE_BAR
};Junio, is this what you meant? It seems to me that there is an extra array to be managed in the latter case. Admittedly, this is a tradeoff against needing to convert the enum to a string when checking config, as you describe [1].
[1] https://lore.kernel.org/git/xmqq7e09hydx.fsf@gitster-ct.c.googlers.com/