On Wed, Sep 09, 2009 at 12:06:21PM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > diff --git a/advice.c b/advice.c
> > new file mode 100644
> > index 0000000..b5216a2
> > --- /dev/null
> > +++ b/advice.c
> > @@ -0,0 +1,25 @@
> > +#include "cache.h"
> > +
> > +int advice_push_nonfastforward = 1;
> > +
> > +static struct {
> > + const char *name;
> > + int *preference;
> > +} advice_config[] = {
> > + { "pushnonfastforward", &advice_push_nonfastforward },
> > +};
>
> Can we have the value inside this struct, instead of having a pointer
> to another variable, and get rid of that variable altogether?
We could, but then callers need some way of indexing into the array.
Which means either:
- a constant offset (like "#define ADVICE_PUSH_NONFASTFORWARD 0"). The
problem with that is you get no compile-time support for making sure
that your index matches the variable you want. I.e., you have:
{ "pushnonfastforward", 1 } /* ADVICE_PUSH_NONFASTFORWARD */
with the position in the array implicitly matching the manually
assigned numbers. We do have precedent in things like diff_colors.
I don't remember if we have ever screwed it up.
- a dynamic offset, like (as you noted):
> If we did so, this part needs to become
>
> if (nonfastforward && check_advice("pushnonfastforward")) {
>
> which would be less efficient, but by definition advices are on the slow
> path, right?
which, as you note, is less efficient. It also turns typo-checking into
a run-time error rather than a compile-time error, which is IMHO a bad
idea. And if you care about such things, it is worse for using something
like ctags to find variable uses.
I went the way I did because it provides compile-time checking, and
because the variable is referred to by name in the table, the matching
is explicit and thus harder to screw up.
One final option would be to get rid of the table altogether. Its
function is to allow you to iterate over all of the variables. Now that
the "advice.all" option has been dropped, the only use is during config
parsing. We could simply unroll that loop to:
if (!strcmp(k, "pushnonfastforward")) {
advice_push_nonfastforward = git_config_bool(var, value);
return 0;
}
if (!strcmp(k, "statushints")) {
advice_status_hints = git_config_bool(var, value);
return 0;
}
as we do in other config parsing.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html