diff mbox series

[4/9] commit-reach: use `size_t` to track indices in `remove_redundant()`

Message ID 20241227-b4-pks-commit-reach-sign-compare-v1-4-07c59c2aa632@pks.im (mailing list archive)
State Accepted
Commit 45843d8f4eb2bbfc73cc361ba9d612d088dc8a4f
Headers show
Series commit-reach: -Wsign-compare follow-ups | expand

Commit Message

Patrick Steinhardt Dec. 27, 2024, 10:46 a.m. UTC
The function `remove_redundant()` gets as input an array of commits as
well as the size of that array and then drops redundant commits from
that array. It then returns either `-1` in case an error occurred, or
the new number of items in the array.

The function receives and returns these sizes with a signed integer,
which causes several warnings with -Wsign-compare. Fix this issue by
consistently using `size_t` to track array indices and splitting up
the returned value into a returned error code and a separate out pointer
for the new computed size.

Note that `get_merge_bases_many()` and related functions still track
array sizes as a signed integer. This will be fixed in a subsequent
commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 commit-reach.c | 53 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

Comments

Justin Tobler Jan. 3, 2025, 1:46 a.m. UTC | #1
On 24/12/27 11:46AM, Patrick Steinhardt wrote:
> The function `remove_redundant()` gets as input an array of commits as
> well as the size of that array and then drops redundant commits from
> that array. It then returns either `-1` in case an error occurred, or
> the new number of items in the array.
> 
> The function receives and returns these sizes with a signed integer,
> which causes several warnings with -Wsign-compare. Fix this issue by
> consistently using `size_t` to track array indices and splitting up
> the returned value into a returned error code and a separate out pointer
> for the new computed size.
> 
> Note that `get_merge_bases_many()` and related functions still track
> array sizes as a signed integer. This will be fixed in a subsequent
> commit.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  commit-reach.c | 53 ++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 30 insertions(+), 23 deletions(-)
> 
> diff --git a/commit-reach.c b/commit-reach.c
> index 9f8b2457bcc12bebf725a5276d1aec467bb7af05..d7f6f1be75e95cc834d60be719e930a77ad0518f 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -212,12 +212,13 @@ int get_octopus_merge_bases(struct commit_list *in, struct commit_list **result)
>  }
>  
>  static int remove_redundant_no_gen(struct repository *r,
> -				   struct commit **array, int cnt)
> +				   struct commit **array,
> +				   size_t cnt, size_t *dedup_cnt)
>  {
>  	struct commit **work;
>  	unsigned char *redundant;
> -	int *filled_index;
> -	int i, j, filled;
> +	size_t *filled_index;
> +	size_t i, j, filled;
>  
>  	CALLOC_ARRAY(work, cnt);
>  	redundant = xcalloc(cnt, 1);
> @@ -267,20 +268,22 @@ static int remove_redundant_no_gen(struct repository *r,
>  	for (i = filled = 0; i < cnt; i++)
>  		if (!redundant[i])
>  			array[filled++] = work[i];
> +	*dedup_cnt = filled;
>  	free(work);
>  	free(redundant);
>  	free(filled_index);
> -	return filled;
> +	return 0;

Previously the return value indicated either a potential error if its
value was negative or the new count otherwise. Since we now want to make
the count a `size_t` to avoid -Wsign-compare warnings, we split the two
concerns and have a separate pointer used to record the count. This
approach is used for each of the `remove_redundant*()` functions. Seems
sensible to me.

-Justin
diff mbox series

Patch

diff --git a/commit-reach.c b/commit-reach.c
index 9f8b2457bcc12bebf725a5276d1aec467bb7af05..d7f6f1be75e95cc834d60be719e930a77ad0518f 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -212,12 +212,13 @@  int get_octopus_merge_bases(struct commit_list *in, struct commit_list **result)
 }
 
 static int remove_redundant_no_gen(struct repository *r,
-				   struct commit **array, int cnt)
+				   struct commit **array,
+				   size_t cnt, size_t *dedup_cnt)
 {
 	struct commit **work;
 	unsigned char *redundant;
-	int *filled_index;
-	int i, j, filled;
+	size_t *filled_index;
+	size_t i, j, filled;
 
 	CALLOC_ARRAY(work, cnt);
 	redundant = xcalloc(cnt, 1);
@@ -267,20 +268,22 @@  static int remove_redundant_no_gen(struct repository *r,
 	for (i = filled = 0; i < cnt; i++)
 		if (!redundant[i])
 			array[filled++] = work[i];
+	*dedup_cnt = filled;
 	free(work);
 	free(redundant);
 	free(filled_index);
-	return filled;
+	return 0;
 }
 
 static int remove_redundant_with_gen(struct repository *r,
-				     struct commit **array, int cnt)
+				     struct commit **array, size_t cnt,
+				     size_t *dedup_cnt)
 {
-	int i, count_non_stale = 0, count_still_independent = cnt;
+	size_t i, count_non_stale = 0, count_still_independent = cnt;
 	timestamp_t min_generation = GENERATION_NUMBER_INFINITY;
 	struct commit **walk_start, **sorted;
 	size_t walk_start_nr = 0, walk_start_alloc = cnt;
-	int min_gen_pos = 0;
+	size_t min_gen_pos = 0;
 
 	/*
 	 * Sort the input by generation number, ascending. This allows
@@ -326,12 +329,12 @@  static int remove_redundant_with_gen(struct repository *r,
 	 * terminate early. Otherwise, we will do the same amount of work
 	 * as before.
 	 */
-	for (i = walk_start_nr - 1; i >= 0 && count_still_independent > 1; i--) {
+	for (i = walk_start_nr; i && count_still_independent > 1; i--) {
 		/* push the STALE bits up to min generation */
 		struct commit_list *stack = NULL;
 
-		commit_list_insert(walk_start[i], &stack);
-		walk_start[i]->object.flags |= STALE;
+		commit_list_insert(walk_start[i - 1], &stack);
+		walk_start[i - 1]->object.flags |= STALE;
 
 		while (stack) {
 			struct commit_list *parents;
@@ -388,10 +391,12 @@  static int remove_redundant_with_gen(struct repository *r,
 	clear_commit_marks_many(walk_start_nr, walk_start, STALE);
 	free(walk_start);
 
-	return count_non_stale;
+	*dedup_cnt = count_non_stale;
+	return 0;
 }
 
-static int remove_redundant(struct repository *r, struct commit **array, int cnt)
+static int remove_redundant(struct repository *r, struct commit **array,
+			    size_t cnt, size_t *dedup_cnt)
 {
 	/*
 	 * Some commit in the array may be an ancestor of
@@ -401,19 +406,17 @@  static int remove_redundant(struct repository *r, struct commit **array, int cnt
 	 * that number.
 	 */
 	if (generation_numbers_enabled(r)) {
-		int i;
-
 		/*
 		 * If we have a single commit with finite generation
 		 * number, then the _with_gen algorithm is preferred.
 		 */
-		for (i = 0; i < cnt; i++) {
+		for (size_t i = 0; i < cnt; i++) {
 			if (commit_graph_generation(array[i]) < GENERATION_NUMBER_INFINITY)
-				return remove_redundant_with_gen(r, array, cnt);
+				return remove_redundant_with_gen(r, array, cnt, dedup_cnt);
 		}
 	}
 
-	return remove_redundant_no_gen(r, array, cnt);
+	return remove_redundant_no_gen(r, array, cnt, dedup_cnt);
 }
 
 static int get_merge_bases_many_0(struct repository *r,
@@ -425,7 +428,8 @@  static int get_merge_bases_many_0(struct repository *r,
 {
 	struct commit_list *list;
 	struct commit **rslt;
-	int cnt, i;
+	size_t cnt, i;
+	int ret;
 
 	if (merge_bases_many(r, one, n, twos, result) < 0)
 		return -1;
@@ -452,8 +456,8 @@  static int get_merge_bases_many_0(struct repository *r,
 	clear_commit_marks(one, all_flags);
 	clear_commit_marks_many(n, twos, all_flags);
 
-	cnt = remove_redundant(r, rslt, cnt);
-	if (cnt < 0) {
+	ret = remove_redundant(r, rslt, cnt, &cnt);
+	if (ret < 0) {
 		free(rslt);
 		return -1;
 	}
@@ -582,7 +586,8 @@  struct commit_list *reduce_heads(struct commit_list *heads)
 	struct commit_list *p;
 	struct commit_list *result = NULL, **tail = &result;
 	struct commit **array;
-	int num_head, i;
+	size_t num_head, i;
+	int ret;
 
 	if (!heads)
 		return NULL;
@@ -603,11 +608,13 @@  struct commit_list *reduce_heads(struct commit_list *heads)
 			p->item->object.flags &= ~STALE;
 		}
 	}
-	num_head = remove_redundant(the_repository, array, num_head);
-	if (num_head < 0) {
+
+	ret = remove_redundant(the_repository, array, num_head, &num_head);
+	if (ret < 0) {
 		free(array);
 		return NULL;
 	}
+
 	for (i = 0; i < num_head; i++)
 		tail = &commit_list_insert(array[i], tail)->next;
 	free(array);