Re: [PATCH 2/4] Add a new function, filter_string_list()

[Michael Haggerty <mhagger@alum.mit.edu>]

On 09/09/2012 11:40 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  Documentation/technical/api-string-list.txt |  8 ++++++++
>>  string-list.c                               | 17 +++++++++++++++++
>>  string-list.h                               |  9 +++++++++
>>  3 files changed, 34 insertions(+)
>>
>> diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt
>> index 3b959a2..15b8072 100644
>> --- a/Documentation/technical/api-string-list.txt
>> +++ b/Documentation/technical/api-string-list.txt
>> @@ -60,6 +60,14 @@ Functions
>>  
>>  * General ones (works with sorted and unsorted lists as well)
>>  
>> +`filter_string_list`::
>> +
>> +	Apply a function to each item in a list, retaining only the
>> +	items for which the function returns true.  If free_util is
>> +	true, call free() on the util members of any items that have
>> +	to be deleted.  Preserve the order of the items that are
>> +	retained.
> 
> In other words, this can safely be used on both sorted and unsorted
> string list.  Good.

Preserving order (while retaining performance) is the main reason for
this function.  Otherwise, unsorted_string_list_delete_item() could be
used in a loop.

>>  `print_string_list`::
>>  
>>  	Dump a string_list to stdout, useful mainly for debugging purposes. It
>> diff --git a/string-list.c b/string-list.c
>> index 110449c..72610ce 100644
>> --- a/string-list.c
>> +++ b/string-list.c
>> @@ -102,6 +102,23 @@ int for_each_string_list(struct string_list *list,
>>  	return ret;
>>  }
>>  
>> +void filter_string_list(struct string_list *list, int free_util,
>> +			string_list_each_func_t fn, void *cb_data)
>> +{
>> +	int src, dst = 0;
>> +	for (src = 0; src < list->nr; src++) {
>> +		if (fn(&list->items[src], cb_data)) {
>> +			list->items[dst++] = list->items[src];
>> +		} else {
>> +			if (list->strdup_strings)
>> +				free(list->items[src].string);
>> +			if (free_util)
>> +				free(list->items[src].util);
>> +		}
>> +	}
>> +	list->nr = dst;
>> +}
>> +
>>  void string_list_clear(struct string_list *list, int free_util)
>>  {
>>  	if (list->items) {
>> diff --git a/string-list.h b/string-list.h
>> index 7e51d03..84996aa 100644
>> --- a/string-list.h
>> +++ b/string-list.h
>> @@ -29,6 +29,15 @@ int for_each_string_list(struct string_list *list,
>>  #define for_each_string_list_item(item,list) \
>>  	for (item = (list)->items; item < (list)->items + (list)->nr; ++item)
>>  
>> +/*
>> + * Apply fn to each item in list, retaining only the ones for which
>> + * the function returns true.  If free_util is true, call free() on
>> + * the util members of any items that have to be deleted.  Preserve
>> + * the order of the items that are retained.
>> + */
>> +void filter_string_list(struct string_list *list, int free_util,
>> +			string_list_each_func_t fn, void *cb_data);
>> +
>>  /* Use these functions only on sorted lists: */
>>  int string_list_has_string(const struct string_list *list, const char *string);
>>  int string_list_find_insert_index(const struct string_list *list, const char *string,
> 
> Having seen that the previous patch introduced a new test helper for
> unit testing (which is a very good idea) and dedicated a new test
> number, I would have expected to see a new test for filtering
> here.

I thought that the code was too trivial to warrant a test, especially
considering that the memory handling aspect of the function can't be
tested very well.  But you've correctly shamed me into adding tests for
this and also for patch 3/4, string_list_remove_duplicates().

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/