[PATCH 2/2] Convert the users for of for_each_string_list to string_list_for_each

[PATCH 2/2] Convert the users for of for_each_string_list to string_list_for_each

[Alex Riesen]

[-- Attachment #1: Type: text/plain, Size: 4767 bytes --]

The macro is suitable for all these cases and will reduce code of
need to just iterate over the items of a string list.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

> On Tue, Jun 29, 2010 at 10:33, Alex Riesen <raa.lkml@gmail.com> wrote:
>> BTW, now that I took a look at it... The iteration over string_list
>> items looks a little overengineered. At least from the point of
>> view of the existing users of the feature. Wouldn't a simple loop
>> be just as simple to use (if not simplier) and faster (no uninlineable
>> function calls and argument preparation and passing needed)?
>>
>> #define string_list_foreach(item,list) \
>>        for (item = (list)->items; item < (list)->items + (list)->nr; ++item)
>>

And this converts existing callers. Removes more than adds.

 notes.c        |   46 ++++++++++++++--------------------------------
 resolve-undo.c |   34 +++++++++++++++-------------------
 2 files changed, 29 insertions(+), 51 deletions(-)

diff --git a/notes.c b/notes.c
index 6ee04e7..4d5ad35 100644
--- a/notes.c
+++ b/notes.c
@@ -877,14 +877,6 @@ void string_list_add_refs_from_colon_sep(struct
string_list *list,
 	strbuf_release(&globbuf);
 }

-static int string_list_add_refs_from_list(struct string_list_item *item,
-					  void *cb)
-{
-	struct string_list *list = cb;
-	string_list_add_refs_by_glob(list, item->string);
-	return 0;
-}
-
 static int notes_display_config(const char *k, const char *v, void *cb)
 {
 	int *load_refs = cb;
@@ -947,30 +939,18 @@ void init_notes(struct notes_tree *t, const char
*notes_ref,
 	load_subtree(t, &root_tree, t->root, 0);
 }

-struct load_notes_cb_data {
-	int counter;
-	struct notes_tree **trees;
-};
-
-static int load_one_display_note_ref(struct string_list_item *item,
-				     void *cb_data)
-{
-	struct load_notes_cb_data *c = cb_data;
-	struct notes_tree *t = xcalloc(1, sizeof(struct notes_tree));
-	init_notes(t, item->string, combine_notes_ignore, 0);
-	c->trees[c->counter++] = t;
-	return 0;
-}
-
 struct notes_tree **load_notes_trees(struct string_list *refs)
 {
+	struct string_list_item *item;
+	int counter = 0;
 	struct notes_tree **trees;
-	struct load_notes_cb_data cb_data;
 	trees = xmalloc((refs->nr+1) * sizeof(struct notes_tree *));
-	cb_data.counter = 0;
-	cb_data.trees = trees;
-	for_each_string_list(load_one_display_note_ref, refs, &cb_data);
-	trees[cb_data.counter] = NULL;
+	string_list_foreach(item, refs) {
+		struct notes_tree *t = xcalloc(1, sizeof(struct notes_tree));
+		init_notes(t, item->string, combine_notes_ignore, 0);
+		trees[counter++] = t;
+	}
+	trees[counter] = NULL;
 	return trees;
 }

@@ -995,10 +975,12 @@ void init_display_notes(struct display_notes_opt *opt)

 	git_config(notes_display_config, &load_config_refs);

-	if (opt && opt->extra_notes_refs)
-		for_each_string_list(string_list_add_refs_from_list,
-				     opt->extra_notes_refs,
-				     &display_notes_refs);
+	if (opt && opt->extra_notes_refs) {
+		struct string_list_item *item;
+		string_list_foreach(item, opt->extra_notes_refs)
+			string_list_add_refs_by_glob(&display_notes_refs,
+						     item->string);
+        }

 	display_notes_trees = load_notes_trees(&display_notes_refs);
 	string_list_clear(&display_notes_refs, 0);
diff --git a/resolve-undo.c b/resolve-undo.c
index 0f50ee0..a3152ff 100644
--- a/resolve-undo.c
+++ b/resolve-undo.c
@@ -28,29 +28,25 @@ void record_resolve_undo(struct index_state
*istate, struct cache_entry *ce)
 	ui->mode[stage - 1] = ce->ce_mode;
 }

-static int write_one(struct string_list_item *item, void *cbdata)
+void resolve_undo_write(struct strbuf *sb, struct string_list *resolve_undo)
 {
-	struct strbuf *sb = cbdata;
-	struct resolve_undo_info *ui = item->util;
-	int i;
+	struct string_list_item *item;
+	string_list_foreach(item, resolve_undo) {
+		struct resolve_undo_info *ui = item->util;
+		int i;

-	if (!ui)
-		return 0;
-	strbuf_addstr(sb, item->string);
-	strbuf_addch(sb, 0);
-	for (i = 0; i < 3; i++)
-		strbuf_addf(sb, "%o%c", ui->mode[i], 0);
-	for (i = 0; i < 3; i++) {
-		if (!ui->mode[i])
+		if (!ui)
 			continue;
-		strbuf_add(sb, ui->sha1[i], 20);
+		strbuf_addstr(sb, item->string);
+		strbuf_addch(sb, 0);
+		for (i = 0; i < 3; i++)
+			strbuf_addf(sb, "%o%c", ui->mode[i], 0);
+		for (i = 0; i < 3; i++) {
+			if (!ui->mode[i])
+				continue;
+			strbuf_add(sb, ui->sha1[i], 20);
+		}
 	}
-	return 0;
-}
-
-void resolve_undo_write(struct strbuf *sb, struct string_list *resolve_undo)
-{
-	for_each_string_list(write_one, resolve_undo, sb);
 }

 struct string_list *resolve_undo_read(const char *data, unsigned long size)
-- 
1.7.1.622.g408a98

[-- Attachment #2: 0002-Convert-the-users-for-of-for_each_string_list-to-stri.diff --]
[-- Type: application/octet-stream, Size: 4250 bytes --]

From 44f4d65476df97f2aeb4149f75e3e807437af4a1 Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Tue, 29 Jun 2010 10:03:41 +0200
Subject: [PATCH 2/2] Convert the users for of for_each_string_list to string_list_for_each

The macro is suitable for all these cases and will reduce code of
need to just iterate over the items of a string list.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 notes.c        |   46 ++++++++++++++--------------------------------
 resolve-undo.c |   34 +++++++++++++++-------------------
 2 files changed, 29 insertions(+), 51 deletions(-)

diff --git a/notes.c b/notes.c
index 6ee04e7..4d5ad35 100644
--- a/notes.c
+++ b/notes.c
@@ -877,14 +877,6 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,
 	strbuf_release(&globbuf);
 }
 
-static int string_list_add_refs_from_list(struct string_list_item *item,
-					  void *cb)
-{
-	struct string_list *list = cb;
-	string_list_add_refs_by_glob(list, item->string);
-	return 0;
-}
-
 static int notes_display_config(const char *k, const char *v, void *cb)
 {
 	int *load_refs = cb;
@@ -947,30 +939,18 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
 	load_subtree(t, &root_tree, t->root, 0);
 }
 
-struct load_notes_cb_data {
-	int counter;
-	struct notes_tree **trees;
-};
-
-static int load_one_display_note_ref(struct string_list_item *item,
-				     void *cb_data)
-{
-	struct load_notes_cb_data *c = cb_data;
-	struct notes_tree *t = xcalloc(1, sizeof(struct notes_tree));
-	init_notes(t, item->string, combine_notes_ignore, 0);
-	c->trees[c->counter++] = t;
-	return 0;
-}
-
 struct notes_tree **load_notes_trees(struct string_list *refs)
 {
+	struct string_list_item *item;
+	int counter = 0;
 	struct notes_tree **trees;
-	struct load_notes_cb_data cb_data;
 	trees = xmalloc((refs->nr+1) * sizeof(struct notes_tree *));
-	cb_data.counter = 0;
-	cb_data.trees = trees;
-	for_each_string_list(load_one_display_note_ref, refs, &cb_data);
-	trees[cb_data.counter] = NULL;
+	string_list_foreach(item, refs) {
+		struct notes_tree *t = xcalloc(1, sizeof(struct notes_tree));
+		init_notes(t, item->string, combine_notes_ignore, 0);
+		trees[counter++] = t;
+	}
+	trees[counter] = NULL;
 	return trees;
 }
 
@@ -995,10 +975,12 @@ void init_display_notes(struct display_notes_opt *opt)
 
 	git_config(notes_display_config, &load_config_refs);
 
-	if (opt && opt->extra_notes_refs)
-		for_each_string_list(string_list_add_refs_from_list,
-				     opt->extra_notes_refs,
-				     &display_notes_refs);
+	if (opt && opt->extra_notes_refs) {
+		struct string_list_item *item;
+		string_list_foreach(item, opt->extra_notes_refs)
+			string_list_add_refs_by_glob(&display_notes_refs,
+						     item->string);
+        }
 
 	display_notes_trees = load_notes_trees(&display_notes_refs);
 	string_list_clear(&display_notes_refs, 0);
diff --git a/resolve-undo.c b/resolve-undo.c
index 0f50ee0..a3152ff 100644
--- a/resolve-undo.c
+++ b/resolve-undo.c
@@ -28,29 +28,25 @@ void record_resolve_undo(struct index_state *istate, struct cache_entry *ce)
 	ui->mode[stage - 1] = ce->ce_mode;
 }
 
-static int write_one(struct string_list_item *item, void *cbdata)
+void resolve_undo_write(struct strbuf *sb, struct string_list *resolve_undo)
 {
-	struct strbuf *sb = cbdata;
-	struct resolve_undo_info *ui = item->util;
-	int i;
+	struct string_list_item *item;
+	string_list_foreach(item, resolve_undo) {
+		struct resolve_undo_info *ui = item->util;
+		int i;
 
-	if (!ui)
-		return 0;
-	strbuf_addstr(sb, item->string);
-	strbuf_addch(sb, 0);
-	for (i = 0; i < 3; i++)
-		strbuf_addf(sb, "%o%c", ui->mode[i], 0);
-	for (i = 0; i < 3; i++) {
-		if (!ui->mode[i])
+		if (!ui)
 			continue;
-		strbuf_add(sb, ui->sha1[i], 20);
+		strbuf_addstr(sb, item->string);
+		strbuf_addch(sb, 0);
+		for (i = 0; i < 3; i++)
+			strbuf_addf(sb, "%o%c", ui->mode[i], 0);
+		for (i = 0; i < 3; i++) {
+			if (!ui->mode[i])
+				continue;
+			strbuf_add(sb, ui->sha1[i], 20);
+		}
 	}
-	return 0;
-}
-
-void resolve_undo_write(struct strbuf *sb, struct string_list *resolve_undo)
-{
-	for_each_string_list(write_one, resolve_undo, sb);
 }
 
 struct string_list *resolve_undo_read(const char *data, unsigned long size)
-- 
1.7.1.622.g408a98

[PATCH 2/2] Convert the users for of for_each_string_list to string_list_for_each

[Alex Riesen]

The macro is suitable for all these cases and will reduce code of
need to just iterate over the items of a string list.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Alex Riesen, Tue, Jun 29, 2010 10:37:17 +0200:
> > On Tue, Jun 29, 2010 at 10:33, Alex Riesen <raa.lkml@gmail.com> wrote:
> >> BTW, now that I took a look at it... The iteration over string_list
> >> items looks a little overengineered. At least from the point of
> >> view of the existing users of the feature. Wouldn't a simple loop
> >> be just as simple to use (if not simplier) and faster (no uninlineable
> >> function calls and argument preparation and passing needed)?
> >>
> >> #define string_list_foreach(item,list) \
> >>        for (item = (list)->items; item < (list)->items + (list)->nr; ++item)
> >>
> 
> And this converts existing callers. Removes more than adds.
> 

Rebased on recent Git master (after Julian Philips patches).

 notes.c        |   46 ++++++++++++++--------------------------------
 resolve-undo.c |   34 +++++++++++++++-------------------
 2 files changed, 29 insertions(+), 51 deletions(-)

diff --git a/notes.c b/notes.c
index 1978244..2d03068 100644
--- a/notes.c
+++ b/notes.c
@@ -877,14 +877,6 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,
 	strbuf_release(&globbuf);
 }
 
-static int string_list_add_refs_from_list(struct string_list_item *item,
-					  void *cb)
-{
-	struct string_list *list = cb;
-	string_list_add_refs_by_glob(list, item->string);
-	return 0;
-}
-
 static int notes_display_config(const char *k, const char *v, void *cb)
 {
 	int *load_refs = cb;
@@ -947,30 +939,18 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
 	load_subtree(t, &root_tree, t->root, 0);
 }
 
-struct load_notes_cb_data {
-	int counter;
-	struct notes_tree **trees;
-};
-
-static int load_one_display_note_ref(struct string_list_item *item,
-				     void *cb_data)
-{
-	struct load_notes_cb_data *c = cb_data;
-	struct notes_tree *t = xcalloc(1, sizeof(struct notes_tree));
-	init_notes(t, item->string, combine_notes_ignore, 0);
-	c->trees[c->counter++] = t;
-	return 0;
-}
-
 struct notes_tree **load_notes_trees(struct string_list *refs)
 {
+	struct string_list_item *item;
+	int counter = 0;
 	struct notes_tree **trees;
-	struct load_notes_cb_data cb_data;
 	trees = xmalloc((refs->nr+1) * sizeof(struct notes_tree *));
-	cb_data.counter = 0;
-	cb_data.trees = trees;
-	for_each_string_list(refs, load_one_display_note_ref, &cb_data);
-	trees[cb_data.counter] = NULL;
+	string_list_foreach(item, refs) {
+		struct notes_tree *t = xcalloc(1, sizeof(struct notes_tree));
+		init_notes(t, item->string, combine_notes_ignore, 0);
+		trees[counter++] = t;
+	}
+	trees[counter] = NULL;
 	return trees;
 }
 
@@ -995,10 +975,12 @@ void init_display_notes(struct display_notes_opt *opt)
 
 	git_config(notes_display_config, &load_config_refs);
 
-	if (opt && opt->extra_notes_refs)
-		for_each_string_list(opt->extra_notes_refs,
-				     string_list_add_refs_from_list,
-				     &display_notes_refs);
+	if (opt && opt->extra_notes_refs) {
+		struct string_list_item *item;
+		string_list_foreach(item, opt->extra_notes_refs)
+			string_list_add_refs_by_glob(&display_notes_refs,
+						     item->string);
+	}
 
 	display_notes_trees = load_notes_trees(&display_notes_refs);
 	string_list_clear(&display_notes_refs, 0);
diff --git a/resolve-undo.c b/resolve-undo.c
index 174ebec..dad5402 100644
--- a/resolve-undo.c
+++ b/resolve-undo.c
@@ -28,29 +28,25 @@ void record_resolve_undo(struct index_state *istate, struct cache_entry *ce)
 	ui->mode[stage - 1] = ce->ce_mode;
 }
 
-static int write_one(struct string_list_item *item, void *cbdata)
+void resolve_undo_write(struct strbuf *sb, struct string_list *resolve_undo)
 {
-	struct strbuf *sb = cbdata;
-	struct resolve_undo_info *ui = item->util;
-	int i;
+	struct string_list_item *item;
+	string_list_foreach(item, resolve_undo) {
+		struct resolve_undo_info *ui = item->util;
+		int i;
 
-	if (!ui)
-		return 0;
-	strbuf_addstr(sb, item->string);
-	strbuf_addch(sb, 0);
-	for (i = 0; i < 3; i++)
-		strbuf_addf(sb, "%o%c", ui->mode[i], 0);
-	for (i = 0; i < 3; i++) {
-		if (!ui->mode[i])
+		if (!ui)
 			continue;
-		strbuf_add(sb, ui->sha1[i], 20);
+		strbuf_addstr(sb, item->string);
+		strbuf_addch(sb, 0);
+		for (i = 0; i < 3; i++)
+			strbuf_addf(sb, "%o%c", ui->mode[i], 0);
+		for (i = 0; i < 3; i++) {
+			if (!ui->mode[i])
+				continue;
+			strbuf_add(sb, ui->sha1[i], 20);
+		}
 	}
-	return 0;
-}
-
-void resolve_undo_write(struct strbuf *sb, struct string_list *resolve_undo)
-{
-   for_each_string_list(resolve_undo, write_one, sb);
 }
 
 struct string_list *resolve_undo_read(const char *data, unsigned long size)
-- 
1.7.1.304.g8446

Re: [PATCH 2/2] Convert the users for of for_each_string_list to string_list_for_each

[Thiago Farina]

On Tue, Jun 29, 2010 at 5:37 AM, Alex Riesen <raa.lkml@gmail.com> wrote:
> The macro is suitable for all these cases

"all these cases" is too vague. Which cases?

> and will reduce code of
> need to just iterate over the items of a string list.
>
A minor comment. There is a typo in the subject:

string_list_for_each -> string_list_foreach.

Also, you didn't convert all the cases. There are usages of
for_each_string_list under builtin/ directory too (I assume it was
intentional).

Re: [PATCH 2/2] Convert the users for of for_each_string_list to string_list_for_each

[Alex Riesen]

On Fri, Jul 2, 2010 at 23:08, Thiago Farina <tfransosi@gmail.com> wrote:
> On Tue, Jun 29, 2010 at 5:37 AM, Alex Riesen <raa.lkml@gmail.com> wrote:
>> The macro is suitable for all these cases
>
> "all these cases" is too vague. Which cases?
>

All the cases of call to for_each_string_list I found and converted.
But you're right, I'll improve.

> Also, you didn't convert all the cases. There are usages of
> for_each_string_list under builtin/ directory too (I assume it was
> intentional).

Er, no. I just failed to use git grep (used the Vim's builtin).
I'll check them out, too.