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

[Junio C Hamano <gitster@pobox.com>]

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.

> 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.