From: shejialuo <shejialuo@gmail.com>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>, Patrick Steinhardt <ps@pks.im>
Subject: [PATCH v2 0/4] enhance string-list API to fix sign compare warnings
Date: Wed, 17 Sep 2025 17:18:00 +0800 [thread overview]
Message-ID: <aMp8yNFiXDyk2hP4@ArchLinux> (raw)
In-Reply-To: <aL21cEM0OcnrKtBW@ArchLinux>
Hi All:
This is a small PATCH to enhance string-list API
"string_list_find_insert_index" which has introduced sign compare
warnings.
---
Changes since v1:
1. Create a new commit which aims at using `bool` for "exact_match"
parameter.
2. Rebase the [PATCH 1/4] and [PATCH 2/4] into a single [PATCH v2 2/4]
commit to avoid confusing the user with the motivation of the
original commit [PATCH 1/4].
3. Enhance the comimt message of [PATCH 2/4] to improve the motivation.
4. Update "i-- > 0" to "i--" for [PATCH 3/4]
Thanks,
Jialuo
shejialuo (4):
string-list: use bool instead of int for "exact_match"
string-list: replace negative index encoding with "exact_match"
parameter
string-list: change "string_list_find_insert_index" return type to
"size_t"
refs: enable sign compare warnings check
add-interactive.c | 7 ++++---
mailmap.c | 10 ++++------
refs.c | 13 ++++---------
string-list.c | 29 ++++++++++++++---------------
string-list.h | 6 +++---
5 files changed, 29 insertions(+), 36 deletions(-)
Range-diff against v1:
1: d3333b4ff6 ! 1: c3786fa386 string-list: allow passing NULL for `get_entry_index`
@@ Metadata
Author: shejialuo <shejialuo@gmail.com>
## Commit message ##
- string-list: allow passing NULL for `get_entry_index`
+ string-list: use bool instead of int for "exact_match"
- Callers of `get_entry_index()` are required to pass a non-NULL
- `exact_match` parameter to receive information about whether an exact
- match is found. However, in some cases, callers only need the index
- position.
-
- Let's allow callers to pass NULL for the `exact_match` parameter
- when they don't need this information, reducing unnecessary variable
- declarations in calling code.
+ The "exact_match" parameter in "get_entry_index" is used to indicate
+ whether a string is found or not, which is fundamentally a true/false
+ value. As we allow the use of bool, let's use bool instead of int to
+ make the function more semantically clear.
Signed-off-by: shejialuo <shejialuo@gmail.com>
## string-list.c ##
+@@ string-list.c: void string_list_init_dup(struct string_list *list)
+ /* if there is no exact match, point to the index where the entry could be
+ * inserted */
+ static size_t get_entry_index(const struct string_list *list, const char *string,
+- int *exact_match)
++ bool *exact_match)
+ {
+ size_t left = 0, right = list->nr;
+ compare_strings_fn cmp = list->cmp ? list->cmp : strcmp;
@@ string-list.c: static size_t get_entry_index(const struct string_list *list, const char *string
else if (compare > 0)
left = middle + 1;
else {
- *exact_match = 1;
-+ if (exact_match)
-+ *exact_match = 1;
++ *exact_match = true;
return middle;
}
}
- *exact_match = 0;
-+ if (exact_match)
-+ *exact_match = 0;
++ *exact_match = false;
return right;
}
+ static size_t add_entry(struct string_list *list, const char *string)
+ {
+- int exact_match = 0;
++ bool exact_match;
+ size_t index = get_entry_index(list, string, &exact_match);
+
+ if (exact_match)
+@@ string-list.c: struct string_list_item *string_list_insert(struct string_list *list, const char
+ void string_list_remove(struct string_list *list, const char *string,
+ int free_util)
+ {
+- int exact_match;
++ bool exact_match;
+ int i = get_entry_index(list, string, &exact_match);
+
+ if (exact_match) {
+@@ string-list.c: void string_list_remove(struct string_list *list, const char *string,
+ }
+ }
+
+-int string_list_has_string(const struct string_list *list, const char *string)
++bool string_list_has_string(const struct string_list *list, const char *string)
+ {
+- int exact_match;
++ bool exact_match;
+ get_entry_index(list, string, &exact_match);
+ return exact_match;
+ }
+@@ string-list.c: 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,
+ int negative_existing_index)
+ {
+- int exact_match;
++ bool exact_match;
+ int index = get_entry_index(list, string, &exact_match);
+ if (exact_match)
+ index = -1 - (negative_existing_index ? index : 0);
+@@ string-list.c: int string_list_find_insert_index(const struct string_list *list, const char *st
+
+ struct string_list_item *string_list_lookup(struct string_list *list, const char *string)
+ {
+- int exact_match, i = get_entry_index(list, string, &exact_match);
++ bool exact_match;
++ size_t i = get_entry_index(list, string, &exact_match);
+ if (!exact_match)
+ return NULL;
+ return list->items + i;
+
+ ## string-list.h ##
+@@ string-list.h: void string_list_remove_empty_items(struct string_list *list, int free_util);
+ /* Use these functions only on sorted lists: */
+
+ /** Determine if the string_list has a given string or not. */
+-int string_list_has_string(const struct string_list *list, const char *string);
++bool 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,
+ int negative_existing_index);
+
2: 104c090d8d ! 2: 7ac8fd69c0 string-list: replace negative index encoding with "exact_match" parameter
@@ Commit message
We would return negative index to indicate exact match by converting the
original positive index to be "-1 - index" in
"string_list_find_insert_index", which requires callers to decode this
- information.
+ information. This approach has several limitations:
- This is bad due to the following reasons:
+ 1. It prevents us from using the full range of size_t, which is
+ necessary for large string list.
+ 2. Using int for indices while other parts of the codebase use size_t
+ creates signed comparison warnings when these values are compared.
- 1. The callers need to convert the negative index back to the original
- positive value, which requires the callers to understand the detail
- of the function.
- 2. As we have to return negative index, we need to specify the return
- type to be `int` instead of `size_t`, which would cause sign compare
- warnings.
+ To address these limitations, change the function to return size_t for
+ the index value and use a separate bool parameter to indicate whether
+ the index refers to an existing entry or an insertion point.
- Refactor "string_list_find_insert_index" to use an output parameter
- "exact_match" for indicating the exact match rather than encoding
- through negative return values.
+ In some cases, the callers of "string_list_find_insert_index" only need
+ the index position and don't care whether an exact match is found.
+ However, "get_entry_index" currently requires a non-NULL "exact_match"
+ parameter, forcing these callers to declare unnecessary variables.
+ Let's allow callers to pass NULL for the "exact_match" parameter when
+ they don't need this information, reducing unnecessary variable
+ declarations in calling code.
Signed-off-by: shejialuo <shejialuo@gmail.com>
@@ add-interactive.c: static void find_unique_prefixes(struct prefix_item_list *lis
static ssize_t find_unique(const char *string, struct prefix_item_list *list)
{
- int index = string_list_find_insert_index(&list->sorted, string, 1);
-+ int exact_match;
++ bool exact_match;
+ int index = string_list_find_insert_index(&list->sorted, string, &exact_match);
struct string_list_item *item;
@@ mailmap.c: void clear_mailmap(struct string_list *map)
- if (i < 0) {
- /* exact match */
- i = -1 - i;
-+ int exact_match;
++ bool exact_match;
+ int i = string_list_find_insert_index(map, string, &exact_match);
+ if (exact_match) {
if (!string[len])
@@ refs.c: const char *find_descendant_ref(const char *dirname,
## string-list.c ##
-@@ string-list.c: int string_list_has_string(const struct string_list *list, const char *string)
+@@ string-list.c: static size_t get_entry_index(const struct string_list *list, const char *string
+ else if (compare > 0)
+ left = middle + 1;
+ else {
+- *exact_match = true;
++ if (exact_match)
++ *exact_match = true;
+ return middle;
+ }
+ }
+
+- *exact_match = false;
++ if (exact_match)
++ *exact_match = false;
+ return right;
+ }
+
+@@ string-list.c: bool 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,
- int negative_existing_index)
-+ int *exact_match)
++ bool *exact_match)
{
-- int exact_match;
+- bool exact_match;
- int index = get_entry_index(list, string, &exact_match);
- if (exact_match)
- index = -1 - (negative_existing_index ? index : 0);
@@ string-list.c: int string_list_has_string(const struct string_list *list, const
## string-list.h ##
@@ string-list.h: void string_list_remove_empty_items(struct string_list *list, int free_util);
/** Determine if the string_list has a given string or not. */
- int string_list_has_string(const struct string_list *list, const char *string);
+ bool 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,
- int negative_existing_index);
-+ int *exact_match);
++ bool *exact_match);
/**
* Insert a new element to the string_list. The returned pointer can
3: 4d4bdf7cda ! 3: 1cf914fab5 string-list: change "string_list_find_insert_index" return type to "size_t"
@@ Commit message
string-list: change "string_list_find_insert_index" return type to "size_t"
As "string_list_find_insert_index" is a simple wrapper of
- "get_entry_index", we could simply change its return type to "size_t".
+ "get_entry_index" and the return type of "get_entry_index" is already
+ "size_t", we could simply change its return type to "size_t".
Update all callers to use size_t variables for storing the return value.
The tricky fix is the loop condition in "mailmap.c" to properly handle
- "size_t" underflow by changing from `0 <= --i` to `i-- > 0`.
+ "size_t" underflow by changing from `0 <= --i` to `i--`.
Remove "DISABLE_SIGN_COMPARE_WARNINGS" from "mailmap.c" as it's no
longer needed with the proper unsigned types.
@@ add-interactive.c
@@ add-interactive.c: static void find_unique_prefixes(struct prefix_item_list *list)
static ssize_t find_unique(const char *string, struct prefix_item_list *list)
{
- int exact_match;
+ bool exact_match;
- int index = string_list_find_insert_index(&list->sorted, string, &exact_match);
+ size_t index = string_list_find_insert_index(&list->sorted, string, &exact_match);
struct string_list_item *item;
@@ mailmap.c
@@ mailmap.c: static struct string_list_item *lookup_prefix(struct string_list *map,
const char *string, size_t len)
{
- int exact_match;
+ bool exact_match;
- int i = string_list_find_insert_index(map, string, &exact_match);
+ size_t i = string_list_find_insert_index(map, string, &exact_match);
if (exact_match) {
@@ mailmap.c: static struct string_list_item *lookup_prefix(struct string_list *map
* real location of the key if one exists.
*/
- while (0 <= --i && i < map->nr) {
-+ while (i-- > 0 && i < map->nr) {
++ while (i-- && i < map->nr) {
int cmp = strncasecmp(map->items[i].string, string, len);
if (cmp < 0)
/*
@@ refs.c: const char *find_descendant_ref(const char *dirname,
## string-list.c ##
-@@ string-list.c: int string_list_has_string(const struct string_list *list, const char *string)
+@@ string-list.c: bool string_list_has_string(const struct string_list *list, const char *string)
return exact_match;
}
-int string_list_find_insert_index(const struct string_list *list, const char *string,
-- int *exact_match)
+- bool *exact_match)
+size_t string_list_find_insert_index(const struct string_list *list, const char *string,
-+ int *exact_match)
++ bool *exact_match)
{
return get_entry_index(list, string, exact_match);
}
@@ string-list.h
@@ string-list.h: void string_list_remove_empty_items(struct string_list *list, int free_util);
/** Determine if the string_list has a given string or not. */
- int string_list_has_string(const struct string_list *list, const char *string);
+ bool 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,
-- int *exact_match);
+- bool *exact_match);
+size_t string_list_find_insert_index(const struct string_list *list, const char *string,
-+ int *exact_match);
++ bool *exact_match);
/**
* Insert a new element to the string_list. The returned pointer can
4: 9f2b55fb41 = 4: 8a445549dd refs: enable sign compare warnings check
--
2.51.0
next prev parent reply other threads:[~2025-09-17 9:17 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-07 16:40 [PATCH 0/4] enhance string-list API to fix sign compare warnings shejialuo
2025-09-07 16:42 ` [PATCH 1/4] string-list: allow passing NULL for `get_entry_index` shejialuo
2025-09-09 6:22 ` Patrick Steinhardt
2025-09-07 16:42 ` [PATCH 2/4] string-list: replace negative index encoding with "exact_match" parameter shejialuo
2025-09-09 6:22 ` Patrick Steinhardt
2025-09-15 12:11 ` shejialuo
2025-09-07 16:42 ` [PATCH 3/4] string-list: change "string_list_find_insert_index" return type to "size_t" shejialuo
2025-09-09 6:23 ` Patrick Steinhardt
2025-09-09 19:21 ` Junio C Hamano
2025-09-10 4:57 ` Patrick Steinhardt
2025-09-07 16:42 ` [PATCH 4/4] refs: enable sign compare warnings check shejialuo
2025-09-09 6:23 ` Patrick Steinhardt
2025-09-07 16:43 ` [PATCH 0/4] enhance string-list API to fix sign compare warnings shejialuo
2025-09-17 9:18 ` shejialuo [this message]
2025-09-17 9:19 ` [PATCH v2 1/4] string-list: use bool instead of int for "exact_match" shejialuo
2025-09-17 9:19 ` [PATCH v2 2/4] string-list: replace negative index encoding with "exact_match" parameter shejialuo
2025-09-23 8:14 ` Patrick Steinhardt
2025-10-05 13:31 ` shejialuo
2025-09-23 9:35 ` Karthik Nayak
2025-09-23 18:48 ` Junio C Hamano
2025-09-24 5:36 ` Jeff King
2025-09-24 13:20 ` Junio C Hamano
2025-09-25 2:50 ` Jeff King
2025-09-25 13:33 ` Junio C Hamano
2025-10-09 5:52 ` Jeff King
2025-10-08 1:49 ` Collin Funk
2025-10-09 5:55 ` Jeff King
2025-10-05 14:11 ` shejialuo
2025-10-05 14:06 ` shejialuo
2025-09-17 9:20 ` [PATCH v2 3/4] string-list: change "string_list_find_insert_index" return type to "size_t" shejialuo
2025-09-23 9:44 ` Karthik Nayak
2025-10-05 9:29 ` shejialuo
2025-09-17 9:20 ` [PATCH v2 4/4] refs: enable sign compare warnings check shejialuo
2025-10-06 6:28 ` [PATCH v3 0/4] enhance string-list API to fix sign compare warnings shejialuo
2025-10-06 6:32 ` [PATCH v3 1/4] string-list: use bool instead of int for "exact_match" shejialuo
2025-10-06 6:32 ` [PATCH v3 2/4] string-list: replace negative index encoding with "exact_match" parameter shejialuo
2025-10-06 6:32 ` [PATCH v3 3/4] string-list: change "string_list_find_insert_index" return type to "size_t" shejialuo
2025-10-09 6:03 ` Jeff King
2025-10-06 6:32 ` [PATCH v3 4/4] refs: enable sign compare warnings check shejialuo
2025-10-06 22:09 ` [PATCH v3 0/4] enhance string-list API to fix sign compare warnings Junio C Hamano
2025-10-08 1:52 ` Collin Funk
2025-10-08 15:56 ` Junio C Hamano
2025-10-08 8:11 ` Karthik Nayak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aMp8yNFiXDyk2hP4@ArchLinux \
--to=shejialuo@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=ps@pks.im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).