enhance "string_list" code and test

Hi All:

I finally finish the version 2. And I don't provide the range-diff due to that I add more commits compared with version 1.

This patch could be organized into three parts:

1. [PATCH v2 1/8]

Fix simple sign warnings of the loop iterator.

2. [PATCH v2 2/8] - [PATCH v2 4/8]

Remove unncessary code, improve the logic and finally enable sign compare warnings check.

3. [PATCH v2 5/8] - [PATCH v2 8/8]

Remove test to the unit test.

---

Changes since v2:

1. [PATCH v3 1/8]: improve the commit message to explain that we would handle a warning in the later commits.
2. [PATCH v3 2/8] and [PATCH v2 3/8]: improve the commit message to add the history background.
3. [PATCH v3 4/8]: improve the commit message to show why the current bianry search algorithm introduces the sign warning and how to change it to fix the sign warning.
4. [PATCH v3 5/8] - [PATCH v3 8/8]: remove list into test helper instead of test itself for reducing the shared state.
5. [PATCH v3 8/8]: improve the commit message to say why we can't delete "test-tool string_list" totally.

Thanks,
Jialuo

shejialuo (8):
string-list: fix sign compare warnings for loop iterator
string-list: remove unused "insert_at" parameter from add_entry
string-list: return index directly when inserting an existing element
string-list: enable sign compare warnings check
u-string-list: move "test_split" into "u-string-list.c"
u-string-list: move "test_split_in_place" to "u-string-list.c"
u-string-list: move "filter string" test to "u-string-list.c"
u-string-list: move "remove duplicates" test to "u-string-list.c"

 Makefile                     |   1 +
 string-list.c                |  48 +++-----
 t/helper/test-string-list.c  |  96 ---------------
 t/meson.build                |   2 +-
 t/t0063-string-list.sh       | 142 ----------------------
 t/unit-tests/u-string-list.c | 227 +++++++++++++++++++++++++++++++++++
 6 files changed, 249 insertions(+), 267 deletions(-)
 delete mode 100755 t/t0063-string-list.sh
 create mode 100644 t/unit-tests/u-string-list.c

Range-diff against v2:
1: 4fb4546525 ! 1: a69eb0f7b8 string-list: fix sign compare warnings for loop iterator
@@ Metadata
## Commit message ##
string-list: fix sign compare warnings for loop iterator

- In "string-list.c", there are six warnings which are emitted by
- "Wsign-compare". And five warnings are caused by the loop iterator type
- mismatch. Let's fix these five warnings by changing the int type to
- size_t type of the loop iterator.
+ There are a couple of "-Wsign-compare" warnings in "string-list.c". Fix
+ trivial ones that result from a mismatched loop iterator type.
+
+ There is a single warning left after these fixes. This warning needs
+ a bit more care and is thus handled in subsequent commits.

2: 3721c92803 ! 2: 35550b3b7d string-list: remove unused "insert_at" parameter from add_entry
@@ Commit message

However, we only use "add_entry" in "string_list_insert" function and we always pass the "-1" for "insert_at" parameter. So, we never use this
- parameter to insert element in a user specified position. Let's delete
- this parameter. If there is any requirement later, we need to use a
- better way to do this.
+ parameter to insert element in a user specified position.
+
+ And we should know why there is such code path in the first place. We
+ used to have another function "string_list_insert_at_index()", which
+ uses the extra "insert_at" parameter. And in f8c4ab611a (string_list:
+ remove string_list_insert_at_index() from its API, 2014-11-24), we
+ remove this function but we don't clean all the code path.
+
+ Let's simply delete this parameter as we'd better use "strmap" for such
+ functionality.

3: fa1d22c93d ! 3: 7877b528e4 string-list: return index directly when inserting an existing element
@@ Commit message

When inserting an existing element, "add_entry" would convert "index" value to "-1-index" to indicate the caller that this element is in the
- list already.
+ list already. However, in "string_list_insert", we would simply convert
+ this to the original positive index without any further action.

- However, in "string_list_insert", we would simply convert this to the
- original positive index without any further action. Let's directly
- return the index as we don't care about whether the element is in the
- list by using "add_entry".
+ In 8fd2cb4069 (Extract helper bits from c-merge-recursive work,
+ 2006-07-25), we create "path-list.c" and then introduce above code path.

- In the future, if we want to let "add_entry" tell the caller, we may add
- "int *exact_match" parameter to "add_entry" instead of converting the
- index to negative to indicate.
+ Let's directly return the index as we don't care about whether the
+ element is in the list by using "add_entry". In the future, if we want
+ to let "add_entry" tell the caller, we may add "int *exact_match"
+ parameter to "add_entry" instead of converting the index to negative to
+ indicate.

4: 02e5c4307b ! 4: 50c4dbff5c string-list: enable sign compare warnings check
@@ Metadata
## Commit message ##
string-list: enable sign compare warnings check

- The only sign compare warning in "string-list" is that we compare the
- index of the int type with the list->nr of unsigned type. We get
- index by calling "get_entry_index", which would always return unsigned
- index.
+ In "add_entry", we call "get_entry_index" function to get the inserted
+ position. However, as the return type of "get_entry_index" function is
+ int, there is a sign compare warning when comparing the index with
+ the list-nr of unsigned type.

- Let's change the return type of "get_entry_index" to be "size_t" by
- slightly modifying the binary search algorithm. Instead of letting
- "left" to be "-1" initially, assign 0 to it.
+ "get_entry_index" would always return unsigned index. However, the
+ current binary search algorithm initializes "left" to be "-1", which
+ necessitates the use of signed int return type.
+
+ The reason why we need to assign "left" to be "-1" is that in the
+ while loop, we increment "left" by 1 to determine whether the loop
+ should end. This design choice, while functional, forces us to use
+ signed arithmetic throughout the function.
+
+ To resolve this sign comparison issue, let's modify the binary search
+ algorithm with the following approach:
+
+ 1. Initialize "left" to 0 instead of -1
+ 2. Use left < right as the loop termination condition instead of
+ left + 1 < right
+ 3. When searching the right part, set left = middle + 1 instead of
+ middle

Then, we could delete "#define DISABLE_SIGN_COMPARE_WARNING" to enable
- sign warnings check for "string-list"
+ sign warnings check for "string-list".

5: 990ce4db0f ! 5: ab2aec5887 u-string-list: move "test_split" into "u-string-list.c"

    @@ t/unit-tests/u-string-list.c (new)
     +		string_list_append(list, arg);
     +}
     +
    -+static void t_string_list_clear(struct string_list *list, int free_util)
    -+{
    -+	string_list_clear(list, free_util);
    -+	cl_assert_equal_p(list->items, NULL);
    -+	cl_assert_equal_i(list->nr, 0);
    -+	cl_assert_equal_i(list->alloc, 0);
    -+}
    -+
     +static void t_string_list_equal(struct string_list *list,
     +				struct string_list *expected_strings)
     +{
    @@ t/unit-tests/u-string-list.c (new)
     +				  expected_strings->items[i].string);
     +}
     +
    -+static void t_string_list_split(struct string_list *list, const char *data,
    -+				int delim, int maxsplit, ...)
    ++static void t_string_list_split(const char *data, int delim, int maxsplit, ...)
     +{
     +	struct string_list expected_strings = STRING_LIST_INIT_DUP;
    ++	struct string_list list = STRING_LIST_INIT_DUP;
     +	va_list ap;
     +	int len;
     +
    @@ t/unit-tests/u-string-list.c (new)
     +	t_vcreate_string_list_dup(&expected_strings, 0, ap);
     +	va_end(ap);
     +
    -+	string_list_clear(list, 0);
    -+	len = string_list_split(list, data, delim, maxsplit);
    ++	string_list_clear(&list, 0);
    ++	len = string_list_split(&list, data, delim, maxsplit);
     +	cl_assert_equal_i(len, expected_strings.nr);
    -+	t_string_list_equal(list, &expected_strings);
    ++	t_string_list_equal(&list, &expected_strings);
     +
     +	string_list_clear(&expected_strings, 0);
    ++	string_list_clear(&list, 0);
     +}
     +
     +void test_string_list__split(void)
     +{
    -+	struct string_list list = STRING_LIST_INIT_DUP;
    -+
    -+	t_string_list_split(&list, "foo:bar:baz", ':', -1, "foo", "bar", "baz", NULL);
    -+	t_string_list_split(&list, "foo:bar:baz", ':', 0, "foo:bar:baz", NULL);
    -+	t_string_list_split(&list, "foo:bar:baz", ':', 1, "foo", "bar:baz", NULL);
    -+	t_string_list_split(&list, "foo:bar:baz", ':', 2, "foo", "bar", "baz", NULL);
    -+	t_string_list_split(&list, "foo:bar:", ':', -1, "foo", "bar", "", NULL);
    -+	t_string_list_split(&list, "", ':', -1, "", NULL);
    -+	t_string_list_split(&list, ":", ':', -1, "", "", NULL);
    -+
    -+	t_string_list_clear(&list, 0);
    ++	t_string_list_split("foo:bar:baz", ':', -1, "foo", "bar", "baz", NULL);
    ++	t_string_list_split("foo:bar:baz", ':', 0, "foo:bar:baz", NULL);
    ++	t_string_list_split("foo:bar:baz", ':', 1, "foo", "bar:baz", NULL);
    ++	t_string_list_split("foo:bar:baz", ':', 2, "foo", "bar", "baz", NULL);
    ++	t_string_list_split("foo:bar:", ':', -1, "foo", "bar", "", NULL);
    ++	t_string_list_split("", ':', -1, "", NULL);
    ++	t_string_list_split(":", ':', -1, "", "", NULL);
     +}
6:  5a02f590df ! 6:  06aca89b57 u-string-list: move "test_split_in_place" to "u-string-list.c"
    @@ t/t0063-string-list.sh: test_description='Test string list functionality'
     
      ## t/unit-tests/u-string-list.c ##
     @@ t/unit-tests/u-string-list.c: void test_string_list__split(void)
    - 
    - 	t_string_list_clear(&list, 0);
    + 	t_string_list_split("", ':', -1, "", NULL);
    + 	t_string_list_split(":", ':', -1, "", "", NULL);
      }
     +
    -+static void t_string_list_split_in_place(struct string_list *list, const char *data,
    -+					 const char *delim, int maxsplit, ...)
    ++static void t_string_list_split_in_place(const char *data, const char *delim,
    ++					 int maxsplit, ...)
     +{
     +	struct string_list expected_strings = STRING_LIST_INIT_DUP;
    ++	struct string_list list = STRING_LIST_INIT_NODUP;
     +	char *string = xstrdup(data);
     +	va_list ap;
     +	int len;
    @@ t/unit-tests/u-string-list.c: void test_string_list__split(void)
     +	t_vcreate_string_list_dup(&expected_strings, 0, ap);
     +	va_end(ap);
     +
    -+	string_list_clear(list, 0);
    -+	len = string_list_split_in_place(list, string, delim, maxsplit);
    ++	string_list_clear(&list, 0);
    ++	len = string_list_split_in_place(&list, string, delim, maxsplit);
     +	cl_assert_equal_i(len, expected_strings.nr);
    -+	t_string_list_equal(list, &expected_strings);
    ++	t_string_list_equal(&list, &expected_strings);
     +
     +	free(string);
     +	string_list_clear(&expected_strings, 0);
    ++	string_list_clear(&list, 0);
     +}
     +
     +void test_string_list__split_in_place(void)
     +{
    -+	struct string_list list = STRING_LIST_INIT_NODUP;
    -+
    -+	t_string_list_split_in_place(&list, "foo:;:bar:;:baz:;:", ":;", -1,
    ++	t_string_list_split_in_place("foo:;:bar:;:baz:;:", ":;", -1,
     +				     "foo", "", "", "bar", "", "", "baz", "", "", "", NULL);
    -+	t_string_list_split_in_place(&list, "foo:;:bar:;:baz", ":;", 0,
    ++	t_string_list_split_in_place("foo:;:bar:;:baz", ":;", 0,
     +				     "foo:;:bar:;:baz", NULL);
    -+	t_string_list_split_in_place(&list, "foo:;:bar:;:baz", ":;", 1,
    ++	t_string_list_split_in_place("foo:;:bar:;:baz", ":;", 1,
     +				     "foo", ";:bar:;:baz", NULL);
    -+	t_string_list_split_in_place(&list, "foo:;:bar:;:baz", ":;", 2,
    ++	t_string_list_split_in_place("foo:;:bar:;:baz", ":;", 2,
     +				     "foo", "", ":bar:;:baz", NULL);
    -+	t_string_list_split_in_place(&list, "foo:;:bar:;:", ":;", -1,
    ++	t_string_list_split_in_place("foo:;:bar:;:", ":;", -1,
     +				     "foo", "", "", "bar", "", "", "", NULL);
    -+
    -+	t_string_list_clear(&list, 0);
     +}
7:  87a6ec722d ! 7:  4cc76a6fb4 u-string-list: move "filter string" test to "u-string-list.c"
    @@ t/unit-tests/u-string-list.c: static void t_vcreate_string_list_dup(struct strin
     +	va_end(ap);
     +}
     +
    - static void t_string_list_clear(struct string_list *list, int free_util)
    ++static void t_string_list_clear(struct string_list *list, int free_util)
    ++{
    ++	string_list_clear(list, free_util);
    ++	cl_assert_equal_p(list->items, NULL);
    ++	cl_assert_equal_i(list->nr, 0);
    ++	cl_assert_equal_i(list->alloc, 0);
    ++}
    ++
    + static void t_string_list_equal(struct string_list *list,
    + 				struct string_list *expected_strings)
      {
    - 	string_list_clear(list, free_util);
     @@ t/unit-tests/u-string-list.c: void test_string_list__split_in_place(void)
    - 
    - 	t_string_list_clear(&list, 0);
    + 	t_string_list_split_in_place("foo:;:bar:;:", ":;", -1,
    + 				     "foo", "", "", "bar", "", "", "", NULL);
      }
     +
     +static int prefix_cb(struct string_list_item *item, void *cb_data)
    @@ t/unit-tests/u-string-list.c: void test_string_list__split_in_place(void)
     +	return starts_with(item->string, prefix);
     +}
     +
    -+static void t_string_list_filter(struct string_list *list,
    -+				 string_list_each_func_t want, void *cb_data, ...)
    ++static void t_string_list_filter(struct string_list *list, ...)
     +{
     +	struct string_list expected_strings = STRING_LIST_INIT_DUP;
    ++	const char *prefix = "y";
     +	va_list ap;
     +
    -+	va_start(ap, cb_data);
    ++	va_start(ap, list);
     +	t_vcreate_string_list_dup(&expected_strings, 0, ap);
     +	va_end(ap);
     +
    -+	filter_string_list(list, 0, want, cb_data);
    ++	filter_string_list(list, 0, prefix_cb, (void *)prefix);
     +	t_string_list_equal(list, &expected_strings);
     +
     +	string_list_clear(&expected_strings, 0);
    @@ t/unit-tests/u-string-list.c: void test_string_list__split_in_place(void)
     +void test_string_list__filter(void)
     +{
     +	struct string_list list = STRING_LIST_INIT_DUP;
    -+	const char *prefix = "y";
     +
     +	t_create_string_list_dup(&list, 0, NULL);
    -+	t_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);
    ++	t_string_list_filter(&list, NULL);
     +
     +	t_create_string_list_dup(&list, 0, "no", NULL);
    -+	t_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);
    ++	t_string_list_filter(&list, NULL);
     +
     +	t_create_string_list_dup(&list, 0, "yes", NULL);
    -+	t_string_list_filter(&list, prefix_cb, (void*)prefix, "yes", NULL);
    ++	t_string_list_filter(&list, "yes", NULL);
     +
     +	t_create_string_list_dup(&list, 0, "no", "yes", NULL);
    -+	t_string_list_filter(&list, prefix_cb, (void*)prefix, "yes", NULL);
    ++	t_string_list_filter(&list, "yes", NULL);
     +
     +	t_create_string_list_dup(&list, 0, "yes", "no", NULL);
    -+	t_string_list_filter(&list, prefix_cb, (void*)prefix, "yes", NULL);
    ++	t_string_list_filter(&list, "yes", NULL);
     +
     +	t_create_string_list_dup(&list, 0, "y1", "y2", NULL);
    -+	t_string_list_filter(&list, prefix_cb, (void*)prefix, "y1", "y2", NULL);
    ++	t_string_list_filter(&list, "y1", "y2", NULL);
     +
     +	t_create_string_list_dup(&list, 0, "y2", "y1", NULL);
    -+	t_string_list_filter(&list, prefix_cb, (void*)prefix, "y2", "y1", NULL);
    ++	t_string_list_filter(&list, "y2", "y1", NULL);
     +
     +	t_create_string_list_dup(&list, 0, "x1", "x2", NULL);
    -+	t_string_list_filter(&list, prefix_cb, (void*)prefix, NULL);
    ++	t_string_list_filter(&list, NULL);
     +
     +	t_string_list_clear(&list, 0);
     +}

8: 55269e7fcb ! 8: b608047d40 u-string-list: move "remove duplicates" test to "u-string-list.c"
@@ Commit message
Also we could simply remove "DISABLE_SIGN_COMPARE_WARNINGS" due to we have already deleted related code.

+ Unfortunately, we cannot totally remove "test-string-list.c" due to that
+ we would test the performance of sorting about string list by executing
+ "test-tool string-list sort" in "p0071-sort.sh".
+

## t/helper/test-string-list.c ##

2.50.0