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)