Re: [GSoC PATCH] rm: fix sign comparison warnings

From

Karthik Nayak <karthik.188@gmail.com>

Date

Mar 13, 2025, 11:25 UTC

Message-ID

<CAOLa=ZTia95Lib6bkz_nWi2BYEteAaOxsrrX9DqLTEz1t02ggA@mail.gmail.com>

In-Reply-To

<xmqqy0x9s8mg.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

> Arnav Bhate <bhatearnav@gmail.com> 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.

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

>> @@ -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) ||