Hi,
On Fri, 6 Jul 2007, Junio C Hamano wrote:
> diff --git a/diff.c b/diff.c
> @@ -1143,6 +1154,82 @@ int diff_filespec_is_binary(struct diff_filespec *one)
> return one->is_binary;
> }
>
> +static struct hunk_header_regexp {
> + char *name;
> + char *regexp;
> + struct hunk_header_regexp *next;
> +} *hunk_header_regexp_list, **hunk_header_regexp_tail;
> +
> +static int hunk_header_config(const char *var, const char *value)
> +{
> + static const char funcname[] = "funcname.";
> + struct hunk_header_regexp *hh;
> +
> + if (prefixcmp(var, funcname))
> + return 0;
> + var += strlen(funcname);
> + for (hh = hunk_header_regexp_list; hh; hh = hh->next)
> + if (!strcmp(var, hh->name)) {
> + free(hh->regexp);
> + hh->regexp = xstrdup(value);
> + return 0;
> + }
Heh. By reordering your code, you could say
if ((hh = hunk_header_regexp(var))) {
free(hh->regexp);
hh->regexp = xstrdup(value);
return 0;
}
> + hh = xcalloc(1, sizeof(*hh));
> + hh->name = xstrdup(var);
> + hh->regexp = xstrdup(value);
> + hh->next = NULL;
> + *hunk_header_regexp_tail = hh;
> + return 0;
> +}
Is that tail expansion not overly complex? Why not just set "hh->next =
hunk_header_regexp_list; hunk_header_regexp_list = hh";
Yes, your code seems correct, but I took some extra cycles to get at that
impression. A "static int parsed_config_for_hunk_headers" would have
helped, instead of reusing _tail for two purposes. And this variable could
be set at the beginning of hunk_header_config(), so that
hunk_header_regexp() is usable from inside hunk_header_config().
> +static const char *hunk_header_regexp(const char *ident)
> +{
> + struct hunk_header_regexp *hh;
> +
> + if (!hunk_header_regexp_tail) {
> + hunk_header_regexp_tail = &hunk_header_regexp_list;
> + git_config(hunk_header_config);
> + }
> + for (hh = hunk_header_regexp_list; hh; hh = hh->next)
> + if (!strcmp(ident, hh->name))
> + return hh->regexp;
> + return NULL;
> +}
Another thing. These long names are a bit inconsistent. In the config, you
name it "funcname". In xdiff, we name them "FUNCNAMES". Yes, here they are
hunk_headers.
Also, since the expressions are not strictly regular expressions, but
lists of them, and with your idea they are even more different, why not
just go for "funcname_list"? It's easier to read, and static anyway.
Rest looks fine to me...
Ciao,
Dscho
-
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
