[PATCH v3 0/8] enhance "string_list" code and test

[shejialuo <shejialuo@gmail.com>]

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.
     
         Signed-off-by: shejialuo <shejialuo@gmail.com>
     
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.
     
         Signed-off-by: shejialuo <shejialuo@gmail.com>
     
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.
     
         Signed-off-by: shejialuo <shejialuo@gmail.com>
     
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".
     
         Signed-off-by: shejialuo <shejialuo@gmail.com>
     
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".
    +
         Signed-off-by: shejialuo <shejialuo@gmail.com>
     
      ## t/helper/test-string-list.c ##
-- 
2.50.0