Re: [PATCH v2 3/8] string-list: return index directly when inserting an existing element
From: Patrick Steinhardt <hidden>
Date: 2025-05-19 07:18:03
On Sun, May 18, 2025 at 11:57:15PM +0800, shejialuo wrote:
quoted hunk ↗ jump to hunk
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. 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 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 <redacted> --- string-list.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)diff --git a/string-list.c b/string-list.c index 8540c29bc9..171cef5dbb 100644 --- a/string-list.c +++ b/string-list.c@@ -40,14 +40,13 @@ static int get_entry_index(const struct string_list *list, const char *string, return right; } -/* returns -1-index if already exists */ static int add_entry(struct string_list *list, const char *string) { int exact_match = 0; int index = get_entry_index(list, string, &exact_match); if (exact_match) - return -1 - index; + return index; ALLOC_GROW(list->items, list->nr+1, list->alloc); if (index < list->nr)
Okay, let's assume that "index == 2" here and we have an exact match. We'd thus return `-1 - 2 == -3`.
quoted hunk ↗ jump to hunk
@@ -65,9 +64,6 @@ struct string_list_item *string_list_insert(struct string_list *list, const char { int index = add_entry(list, string); - if (index < 0) - index = -1 - index; - return list->items + index; }
So we'd now realize that `index < 0` and thus calculate `-1 - -3 == 2`, which is the original index indeed. So this is a nice simplification that retains the original behaviour indeed. I think we could simplify the code even further by inlining `get_entry_index()` now that `string_list_insert()` is a trivial wrapper around it. But I'll leave it up to you whether we want to do it or not. Patrick