Arnav Bhate <bhatearnav@xxxxxxxxx> writes:
> 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.
>
Of course, that's why I said 'something like' :) This is only a guidance,
the final changes are left to you.
>>>> @@ -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)
Attachment:
signature.asc
Description: PGP signature
