Karthik Nayak <karthik.188@xxxxxxxxx> writes:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
>> Arnav Bhate <bhatearnav@xxxxxxxxx> writes:
>>
>>> static int get_ours_cache_pos(const char *path, int pos)
>>> {
>>> - int i = -pos - 1;
>>> + /*
>>> + * This function is only called when pos < 0, so -pos - 1 is
>>> + * greater than or equal to 0, so it can be safely be stored in
>>> + * an unsigned int.
>>> + */
>>> + unsigned int i = -pos - 1;
>>
>> "Can be safely stored", sure.
>>
>> But so is "int i" perfectly adequate to hold such a value, no?
>>
>> This is one of the many instances that demonstrate why the
>> "-Wsign-compare" warning is of dubious value, and invites worse code
>> than necessary.
>
> I have to agree. I think it would a bit cleaner to actually change the
> functions argument type itself. Perhaps, something like:
>
> -- >8 --
>
> diff --git a/builtin/rm.c b/builtin/rm.c
> index 12ae086a55..79e47d6e9e 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -40,10 +40,8 @@ static struct {
> } *entry;
> } list;
>
> -static int get_ours_cache_pos(const char *path, int pos)
> +static int get_ours_cache_pos(const char *path, unsigned int i)
> {
> - int i = -pos - 1;
> -
> while ((i < the_repository->index->cache_nr) &&
> !strcmp(the_repository->index->cache[i]->name, path)) {
> if (ce_stage(the_repository->index->cache[i]) == 2)
> return i;
> @@ -83,7 +81,7 @@ static void submodules_absorb_gitdir_if_needed(void)
>
> pos = index_name_pos(the_repository->index, name, strlen(name));
> if (pos < 0) {
> - pos = get_ours_cache_pos(name, pos);
> + pos = get_ours_cache_pos(name, -pos - 1);
> if (pos < 0)
> continue;
> }
> @@ -131,7 +129,7 @@ static int check_local_mod(struct object_id *head,
> int index_only)
> * Skip unmerged entries except for populated submodules
> * that could lose history when removed.
> */
> - pos = get_ours_cache_pos(name, pos);
> + pos = get_ours_cache_pos(name, -pos - 1);
> if (pos < 0)
> continue;
This is a good option, I think, but perhaps 'i' should be renamed to
something more descriptive.
>>> @@ -58,7 +62,7 @@ static void print_error_files(struct string_list *files_list,
>>> int *errs)
>>> {
>>> if (files_list->nr) {
>>> - int i;
>>> + unsigned int i;
>>> struct strbuf err_msg = STRBUF_INIT;
>>>
>>> strbuf_addstr(&err_msg, main_msg);
>>> @@ -271,6 +275,7 @@ int cmd_rm(int argc,
>>> {
>>> struct lock_file lock_file = LOCK_INIT;
>>> int i, ret = 0;
>>> + unsigned int j;
>>> struct pathspec pathspec;
>>> char *seen;
>>>
>>> @@ -314,8 +319,8 @@ int cmd_rm(int argc,
>>> if (pathspec_needs_expanded_index(the_repository->index, &pathspec))
>>> ensure_full_index(the_repository->index);
>>>
>>> - for (i = 0; i < the_repository->index->cache_nr; i++) {
>>> - const struct cache_entry *ce = the_repository->index->cache[i];
>>> + for (j = 0; j < the_repository->index->cache_nr; j++) {
>>> + const struct cache_entry *ce = the_repository->index->cache[j];
>>>
>>> if (!include_sparse &&
>>> (ce_skip_worktree(ce) ||
--
Regards,
Arnav Bhate
(He/Him)
