{"thread":{"id":"24228","subject":"[PATCH 1/2] Add a string_list_foreach macro","startedAt":"2010-06-29T08:35:15Z","lastAt":"2010-07-06T02:35:50Z","messageCount":5,"participants":["Alex Riesen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"144450","messageId":"AANLkTilj7MiqiCmptDw0PLM5QqKZOOSZnSsxMlELS_5_@mail.gmail.com","threadId":"24228","inReplyTo":null,"subject":"[PATCH 1/2] Add a string_list_foreach macro","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2010-06-29T08:35:15Z","receivedAt":"2010-06-29T08:35:15Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"This is more lightweight than for_each_string_list function with\ncallback function and a cookie argument.\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n\nOn Tue, Jun 29, 2010 at 10:33, Alex Riesen <raa.lkml@gmail.com> wrote:\n> BTW, now that I took a look at it... The iteration over string_list\n> items looks a little overengineered. At least from the point of\n> view of the existing users of the feature. Wouldn't a simple loop\n> be just as simple to use (if not simplier) and faster (no uninlineable\n> function calls and argument preparation and passing needed)?\n>\n> #define string_list_foreach(item,list) \\\n>        for (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n>\n\n string-list.h |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/string-list.h b/string-list.h\nindex 63b69c8..188d087 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -24,6 +24,8 @@ void string_list_clear_func(struct string_list\n*list, string_list_clear_func_t c\n typedef int (*string_list_each_func_t)(struct string_list_item *, void *);\n int for_each_string_list(string_list_each_func_t,\n \t\t\t struct string_list *list, void *cb_data);\n+#define string_list_foreach(item,list) \\\n+\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n\n /* Use these functions only on sorted lists: */\n int string_list_has_string(const struct string_list *list, const char *string);\n-- \n1.7.1.622.g408a98\n\n\nFrom b2e7d2dce0cb8a6b50af5ba04ededfb342643a90 Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen <raa.lkml@gmail.com>\nDate: Tue, 29 Jun 2010 10:02:44 +0200\nSubject: [PATCH 1/2] Add a string_list_foreach macro\n\nThis is more lightweight than for_each_string_list function with\ncallback function and a cookie argument.\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n string-list.h |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/string-list.h b/string-list.h\nindex 63b69c8..188d087 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -24,6 +24,8 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c\n typedef int (*string_list_each_func_t)(struct string_list_item *, void *);\n int for_each_string_list(string_list_each_func_t,\n \t\t\t struct string_list *list, void *cb_data);\n+#define string_list_foreach(item,list) \\\n+\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n \n /* Use these functions only on sorted lists: */\n int string_list_has_string(const struct string_list *list, const char *string);\n-- \n1.7.1.622.g408a98\n\n"},{"id":"144719","messageId":"20100702205417.GA4941@blimp.localdomain","threadId":"24228","inReplyTo":"AANLkTilj7MiqiCmptDw0PLM5QqKZOOSZnSsxMlELS_5_@mail.gmail.com","subject":"[PATCH 1/2] Add a string_list_foreach macro","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2010-07-02T20:54:17Z","receivedAt":"2010-07-02T20:54:17Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"This is more lightweight than for_each_string_list function with\ncallback function and a cookie argument.\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n\nAlex Riesen, Tue, Jun 29, 2010 10:35:15 +0200:\n> On Tue, Jun 29, 2010 at 10:33, Alex Riesen <raa.lkml@gmail.com> wrote:\n> > BTW, now that I took a look at it... The iteration over string_list\n> > items looks a little overengineered. At least from the point of\n> > view of the existing users of the feature. Wouldn't a simple loop\n> > be just as simple to use (if not simplier) and faster (no uninlineable\n> > function calls and argument preparation and passing needed)?\n> >\n> > #define string_list_foreach(item,list) \\\n> >        for (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n> >\n\nRebased on current head (after Julian Philips patches).\n\n string-list.h |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/string-list.h b/string-list.h\nindex 680d600..acf0450 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -24,6 +24,8 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c\n typedef int (*string_list_each_func_t)(struct string_list_item *, void *);\n int for_each_string_list(struct string_list *list,\n \t\t\t string_list_each_func_t, void *cb_data);\n+#define string_list_foreach(item,list) \\\n+\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n \n /* Use these functions only on sorted lists: */\n int string_list_has_string(const struct string_list *list, const char *string);\n-- \n1.7.1.304.g8446\n"},{"id":"144743","messageId":"20100703124004.GA5511@blimp.localdomain","threadId":"24228","inReplyTo":"20100702205417.GA4941@blimp.localdomain","subject":"[PATCH 1/2] Add a for_each_string_list_item macro","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2010-07-03T12:40:04Z","receivedAt":"2010-07-03T12:40:04Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"This is more lightweight than a call to for_each_string_list function with\ncallback function and a cookie argument.\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n\nAlex Riesen, Fri, Jul 02, 2010 22:54:17 +0200:\n> This is more lightweight than for_each_string_list function with\n> callback function and a cookie argument.\n> \n> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>\n> ---\n> \n> Alex Riesen, Tue, Jun 29, 2010 10:35:15 +0200:\n> > On Tue, Jun 29, 2010 at 10:33, Alex Riesen <raa.lkml@gmail.com> wrote:\n> > > BTW, now that I took a look at it... The iteration over string_list\n> > > items looks a little overengineered. At least from the point of\n> > > view of the existing users of the feature. Wouldn't a simple loop\n> > > be just as simple to use (if not simplier) and faster (no uninlineable\n> > > function calls and argument preparation and passing needed)?\n> > >\n> > > #define string_list_foreach(item,list) \\\n> > >        for (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n> > >\n> \n> Rebased on current head (after Julian Philips patches).\n> \n\nChanged the macro name to make it look like the for_each* functions.\n\n string-list.h |    4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/string-list.h b/string-list.h\nindex 680d600..a37cae5 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -20,10 +20,12 @@ void string_list_clear(struct string_list *list, int free_util);\n typedef void (*string_list_clear_func_t)(void *p, const char *str);\n void string_list_clear_func(struct string_list *list, string_list_clear_func_t clearfunc);\n \n-/* Use this function to iterate over each item */\n+/* Use this function or the macro below to iterate over each item */\n typedef int (*string_list_each_func_t)(struct string_list_item *, void *);\n int for_each_string_list(struct string_list *list,\n \t\t\t string_list_each_func_t, void *cb_data);\n+#define for_each_string_list_item(item,list) \\\n+\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n \n /* Use these functions only on sorted lists: */\n int string_list_has_string(const struct string_list *list, const char *string);\n-- \n1.7.1.304.g8446\n"},{"id":"144744","messageId":"20100703124154.GB5511@blimp.localdomain","threadId":"24228","inReplyTo":"20100703124004.GA5511@blimp.localdomain","subject":"[PATCH 2/2] Convert the users of for_each_string_list to for_each_string_list_item macro","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2010-07-03T12:41:54Z","receivedAt":"2010-07-03T12:41:54Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"The rule for selecting the candidates for conversion is: if the callback\nfunction returns only 0 (the condition for for_each_string_list to exit\nearly), than it can be safely converted to the macro.\n\nA notable exception are the callers in builtin/remote.c. If converted, the\nreadability in the file will suffer greately. Besides, the code is not very\nperformance critical (at the moment, at least): it does output formatting of\nthe list of remotes.\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n\n---\n\n builtin/fetch.c    |   42 +++++++++++++-----------------------------\n builtin/ls-files.c |   45 ++++++++++++++++++++++-----------------------\n notes.c            |   46 ++++++++++++++--------------------------------\n resolve-undo.c     |   34 +++++++++++++++-------------------\n 4 files changed, 64 insertions(+), 103 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 6eb1dfe..b0bfaa9 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -544,40 +544,14 @@ static int will_fetch(struct ref **head, const unsigned char *sha1)\n \treturn 0;\n }\n \n-struct tag_data {\n-\tstruct ref **head;\n-\tstruct ref ***tail;\n-};\n-\n-static int add_to_tail(struct string_list_item *item, void *cb_data)\n-{\n-\tstruct tag_data *data = (struct tag_data *)cb_data;\n-\tstruct ref *rm = NULL;\n-\n-\t/* We have already decided to ignore this item */\n-\tif (!item->util)\n-\t\treturn 0;\n-\n-\trm = alloc_ref(item->string);\n-\trm->peer_ref = alloc_ref(item->string);\n-\thashcpy(rm->old_sha1, item->util);\n-\n-\t**data->tail = rm;\n-\t*data->tail = &rm->next;\n-\n-\treturn 0;\n-}\n-\n static void find_non_local_tags(struct transport *transport,\n \t\t\tstruct ref **head,\n \t\t\tstruct ref ***tail)\n {\n \tstruct string_list existing_refs = { NULL, 0, 0, 0 };\n \tstruct string_list remote_refs = { NULL, 0, 0, 0 };\n-\tstruct tag_data data;\n \tconst struct ref *ref;\n \tstruct string_list_item *item = NULL;\n-\tdata.head = head; data.tail = tail;\n \n \tfor_each_ref(add_existing, &existing_refs);\n \tfor (ref = transport_get_remote_refs(transport); ref; ref = ref->next) {\n@@ -631,10 +605,20 @@ static void find_non_local_tags(struct transport *transport,\n \t\titem->util = NULL;\n \n \t/*\n-\t * For all the tags in the remote_refs string list, call\n-\t * add_to_tail to add them to the list of refs to be fetched\n+\t * For all the tags in the remote_refs string list,\n+\t * add them to the list of refs to be fetched\n \t */\n-\tfor_each_string_list(&remote_refs, add_to_tail, &data);\n+\tfor_each_string_list_item(item, &remote_refs) {\n+\t\t/* Unless we have already decided to ignore this item... */\n+\t\tif (item->util)\n+\t\t{\n+\t\t\tstruct ref *rm = alloc_ref(item->string);\n+\t\t\trm->peer_ref = alloc_ref(item->string);\n+\t\t\thashcpy(rm->old_sha1, item->util);\n+\t\t\t**tail = rm;\n+\t\t\t*tail = &rm->next;\n+\t\t}\n+\t}\n \n \tstring_list_clear(&remote_refs, 0);\n }\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 1b9b8a8..cf6ab03 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -164,33 +164,32 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)\n \twrite_name(ce->name, ce_namelen(ce));\n }\n \n-static int show_one_ru(struct string_list_item *item, void *cbdata)\n-{\n-\tconst char *path = item->string;\n-\tstruct resolve_undo_info *ui = item->util;\n-\tint i, len;\n-\n-\tlen = strlen(path);\n-\tif (len < max_prefix_len)\n-\t\treturn 0; /* outside of the prefix */\n-\tif (!match_pathspec(pathspec, path, len, max_prefix_len, ps_matched))\n-\t\treturn 0; /* uninterested */\n-\tfor (i = 0; i < 3; i++) {\n-\t\tif (!ui->mode[i])\n-\t\t\tcontinue;\n-\t\tprintf(\"%s%06o %s %d\\t\", tag_resolve_undo, ui->mode[i],\n-\t\t       find_unique_abbrev(ui->sha1[i], abbrev),\n-\t\t       i + 1);\n-\t\twrite_name(path, len);\n-\t}\n-\treturn 0;\n-}\n-\n static void show_ru_info(void)\n {\n+\tstruct string_list_item *item;\n+\n \tif (!the_index.resolve_undo)\n \t\treturn;\n-\tfor_each_string_list(the_index.resolve_undo, show_one_ru, NULL);\n+\n+\tfor_each_string_list_item(item, the_index.resolve_undo) {\n+\t\tconst char *path = item->string;\n+\t\tstruct resolve_undo_info *ui = item->util;\n+\t\tint i, len;\n+\n+\t\tlen = strlen(path);\n+\t\tif (len < max_prefix_len)\n+\t\t\tcontinue; /* outside of the prefix */\n+\t\tif (!match_pathspec(pathspec, path, len, max_prefix_len, ps_matched))\n+\t\t\tcontinue; /* uninterested */\n+\t\tfor (i = 0; i < 3; i++) {\n+\t\t\tif (!ui->mode[i])\n+\t\t\t\tcontinue;\n+\t\t\tprintf(\"%s%06o %s %d\\t\", tag_resolve_undo, ui->mode[i],\n+\t\t\t       find_unique_abbrev(ui->sha1[i], abbrev),\n+\t\t\t       i + 1);\n+\t\t\twrite_name(path, len);\n+\t\t}\n+\t}\n }\n \n static void show_files(struct dir_struct *dir)\ndiff --git a/notes.c b/notes.c\nindex 1978244..7fd2035 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -877,14 +877,6 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,\n \tstrbuf_release(&globbuf);\n }\n \n-static int string_list_add_refs_from_list(struct string_list_item *item,\n-\t\t\t\t\t  void *cb)\n-{\n-\tstruct string_list *list = cb;\n-\tstring_list_add_refs_by_glob(list, item->string);\n-\treturn 0;\n-}\n-\n static int notes_display_config(const char *k, const char *v, void *cb)\n {\n \tint *load_refs = cb;\n@@ -947,30 +939,18 @@ void init_notes(struct notes_tree *t, const char *notes_ref,\n \tload_subtree(t, &root_tree, t->root, 0);\n }\n \n-struct load_notes_cb_data {\n-\tint counter;\n-\tstruct notes_tree **trees;\n-};\n-\n-static int load_one_display_note_ref(struct string_list_item *item,\n-\t\t\t\t     void *cb_data)\n-{\n-\tstruct load_notes_cb_data *c = cb_data;\n-\tstruct notes_tree *t = xcalloc(1, sizeof(struct notes_tree));\n-\tinit_notes(t, item->string, combine_notes_ignore, 0);\n-\tc->trees[c->counter++] = t;\n-\treturn 0;\n-}\n-\n struct notes_tree **load_notes_trees(struct string_list *refs)\n {\n+\tstruct string_list_item *item;\n+\tint counter = 0;\n \tstruct notes_tree **trees;\n-\tstruct load_notes_cb_data cb_data;\n \ttrees = xmalloc((refs->nr+1) * sizeof(struct notes_tree *));\n-\tcb_data.counter = 0;\n-\tcb_data.trees = trees;\n-\tfor_each_string_list(refs, load_one_display_note_ref, &cb_data);\n-\ttrees[cb_data.counter] = NULL;\n+\tfor_each_string_list_item(item, refs) {\n+\t\tstruct notes_tree *t = xcalloc(1, sizeof(struct notes_tree));\n+\t\tinit_notes(t, item->string, combine_notes_ignore, 0);\n+\t\ttrees[counter++] = t;\n+\t}\n+\ttrees[counter] = NULL;\n \treturn trees;\n }\n \n@@ -995,10 +975,12 @@ void init_display_notes(struct display_notes_opt *opt)\n \n \tgit_config(notes_display_config, &load_config_refs);\n \n-\tif (opt && opt->extra_notes_refs)\n-\t\tfor_each_string_list(opt->extra_notes_refs,\n-\t\t\t\t     string_list_add_refs_from_list,\n-\t\t\t\t     &display_notes_refs);\n+\tif (opt && opt->extra_notes_refs) {\n+\t\tstruct string_list_item *item;\n+\t\tfor_each_string_list_item(item, opt->extra_notes_refs)\n+\t\t\tstring_list_add_refs_by_glob(&display_notes_refs,\n+\t\t\t\t\t\t     item->string);\n+\t}\n \n \tdisplay_notes_trees = load_notes_trees(&display_notes_refs);\n \tstring_list_clear(&display_notes_refs, 0);\ndiff --git a/resolve-undo.c b/resolve-undo.c\nindex 174ebec..72b4612 100644\n--- a/resolve-undo.c\n+++ b/resolve-undo.c\n@@ -28,29 +28,25 @@ void record_resolve_undo(struct index_state *istate, struct cache_entry *ce)\n \tui->mode[stage - 1] = ce->ce_mode;\n }\n \n-static int write_one(struct string_list_item *item, void *cbdata)\n+void resolve_undo_write(struct strbuf *sb, struct string_list *resolve_undo)\n {\n-\tstruct strbuf *sb = cbdata;\n-\tstruct resolve_undo_info *ui = item->util;\n-\tint i;\n+\tstruct string_list_item *item;\n+\tfor_each_string_list_item(item, resolve_undo) {\n+\t\tstruct resolve_undo_info *ui = item->util;\n+\t\tint i;\n \n-\tif (!ui)\n-\t\treturn 0;\n-\tstrbuf_addstr(sb, item->string);\n-\tstrbuf_addch(sb, 0);\n-\tfor (i = 0; i < 3; i++)\n-\t\tstrbuf_addf(sb, \"%o%c\", ui->mode[i], 0);\n-\tfor (i = 0; i < 3; i++) {\n-\t\tif (!ui->mode[i])\n+\t\tif (!ui)\n \t\t\tcontinue;\n-\t\tstrbuf_add(sb, ui->sha1[i], 20);\n+\t\tstrbuf_addstr(sb, item->string);\n+\t\tstrbuf_addch(sb, 0);\n+\t\tfor (i = 0; i < 3; i++)\n+\t\t\tstrbuf_addf(sb, \"%o%c\", ui->mode[i], 0);\n+\t\tfor (i = 0; i < 3; i++) {\n+\t\t\tif (!ui->mode[i])\n+\t\t\t\tcontinue;\n+\t\t\tstrbuf_add(sb, ui->sha1[i], 20);\n+\t\t}\n \t}\n-\treturn 0;\n-}\n-\n-void resolve_undo_write(struct strbuf *sb, struct string_list *resolve_undo)\n-{\n-   for_each_string_list(resolve_undo, write_one, sb);\n }\n \n struct string_list *resolve_undo_read(const char *data, unsigned long size)\n-- \n1.7.1.304.g8446\n"},{"id":"144862","messageId":"7v4ogd8hh5.fsf@alter.siamese.dyndns.org","threadId":"24228","inReplyTo":"20100703124154.GB5511@blimp.localdomain","subject":"Re: [PATCH 2/2] Convert the users of for_each_string_list to for_each_string_list_item macro","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-06T02:35:50Z","receivedAt":"2010-07-06T02:35:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> writes:\n\n> The rule for selecting the candidates for conversion is: if the callback\n> function returns only 0 (the condition for for_each_string_list to exit\n> early), than it can be safely converted to the macro.\n>\n> A notable exception are the callers in builtin/remote.c. If converted, the\n> readability in the file will suffer greately. Besides, the code is not very\n> performance critical (at the moment, at least): it does output formatting of\n> the list of remotes.\n>\n> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>\n\nBoth patches look very sane.  Thanks.\n"}]}