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