Re: [PATCH v2 2/4] string-list: replace negative index encoding with "exact_match" parameter
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Sep 23, 2025, 09:35 UTC
- Message-ID
- <CAOLa=ZShms1D-cq=x04dtT2ULTVE3ZDo8DODFnJRP2wcJz0EgQ@mail.gmail.com>
- In-Reply-To
- <aMp9OtXLfRw7dEwA@ArchLinux>
shejialuo <shejialuo@gmail.com> writes:
Show 5 quoted lines
> 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. This approach has several limitations: >
Nit: It would be nice to start by explaining what "string_list_find_insert_index" does and then talking about the negative index. Perhaps something like:
The `string_list_find_insert_index()` function is used to determine the correct insertion index for a new string within the string list. The function also doubles up to convey if the string is already existing in the list, this is done by returning a negative index "-1 -index". Users are expected to decode this information.
Show 17 quoted lines
> 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. > > 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. > > 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. >
Makes sense, and much cleaner..
Show 29 quoted lines
> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---
> add-interactive.c | 7 ++++---
> mailmap.c | 7 +++----
> refs.c | 2 +-
> string-list.c | 14 ++++++--------
> string-list.h | 2 +-
> 5 files changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/add-interactive.c b/add-interactive.c
> index 3e692b47ec..7c0fd3d218 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -221,7 +221,8 @@ static void find_unique_prefixes(struct prefix_item_list *list)
>
> static ssize_t find_unique(const char *string, struct prefix_item_list *list)
> {
> - int index = string_list_find_insert_index(&list->sorted, string, 1);
> + bool exact_match;
> + int index = string_list_find_insert_index(&list->sorted, string, &exact_match);
> struct string_list_item *item;
>
> if (list->items.nr != list->sorted.nr)
> @@ -229,8 +230,8 @@ static ssize_t find_unique(const char *string, struct prefix_item_list *list)
> " vs %"PRIuMAX")",
> (uintmax_t)list->items.nr, (uintmax_t)list->sorted.nr);
>
> - if (index < 0)
> - item = list->sorted.items[-1 - index].util;Thanks for this, this is so confusing to read if one doesn't know that the incoming information is encoded with a special format.
Rest of the patch looks great.
[snip]