Re: [PATCH 1/5] string-list: fix sign compare warnings

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 22, 2025, 21:02 UTC
Message-ID: <xmqqy0vr3o6d.fsf@gitster.g>
In-Reply-To: <aAett8cJuDJ_FSdw@ArchLinux>

shejialuo <shejialuo@gmail.com> writes:

> However, for "string-list.c::add_entry" function, we compare the `index`
> of the `int` type with the `list->nr` of unsigned type. It seems that
> we could just simply convert the type of `index` from `int` to
> `size_t`. But actually this is a correct behavior.

Sorry, but I am lost by the last sentence.

"this" that is a correct behavior refers to...? That the incoming parameter insert_at and the local variable index are both of signed integer?

> We would set the `index` value by checking whether `insert_at` is -1.
> If not, we would set `index` to be `insert_at`, otherwise we would use
> "get_entry_index` to find the inserted position.

To rephrase the above (simply because the above is literal English translation from what C says), the caller either can pass -1 to mean "find an appropriate location in the list to keep it sorted", or an index into the list->items[] array to specify exactly where the item should be inserted.

Naturally, insert_at must be either -1 (auto), or between 0 (i.e. the candidate is smaller than anything in the list) and list->nr (i.e. the candidate is larger than everything in the list) inclusive. Any other value is invalid. I think that is a more appropriate thing to say than ...

> What if the caller passes a negative value except "-1", the compiler
> would convert the `index` to be a positive value which would make the
> `if` statement be false to avoid moving array. However, we would
> definitely encounter trouble when setting the inserted item.

... this paragraph. Not moving is _not_ avoiding problem, so it is immaterial. The lack of valid range check before using the index is.

Show 6 quoted lines

> And we only call "add_entry" in "string_list_insert" function, and we
> simply pass "-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 may use a better
> way to do this. And then we could safely convert the index to be
> `size_t` when comparing.

Good. As we only use the "auto" setting with this code now, as long as get_entry_index() returns a value between 0 and list->nr, the lack of such range checking in the original code no longer is an issue.

Having said that, in the longer run, get_entry_index() would want to return size_t simply because it is returning a value between 0 and list->nr, whose type is size_t. left/mid/right variables also need to become size_t and the loop initialization may have to be tweaked (since the current code strangely starts left with -1 which would never be the index into the array), but fixing that should probably make the loop easier to read, which is a bonus.

And add_entry(), since it needs to do the usual -1-pos dance to indicate where things would have been returned, would return ssize_t---or better yet, it can just turned into returning size_t with an extra out parameter (just like the exact_match out parameter get_entry_index() has) to indicate if we already had the same item in the list already. It is perfectly fine to leave it outside the scope of this series, but if you are tweaking all the callers of add_entry() anyway in this step, you may want to bite the bullet and just go all the way.

Thanks.