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

[shejialuo <shejialuo@gmail.com>]

On Tue, Apr 22, 2025 at 02:02:18PM -0700, Junio C Hamano wrote:
> 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?
> 

Well, please ignore. I think I made a mistake. I should not say this is
correct but say that this is not harmful.

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

Yes, thanks for the hint.

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

Yes, that's right. Ideally, we should check whether the index is OK. And
I somehow think that I should not make the commit message complicate. I
should just say we should remove "insert_at" function in a separate
commit and then fix sign comparing warnings.

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

That's right. And I would clean the code a bit more.

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

I agree. I'll add more patches to clean.

> Thanks.

Thanks for the review.