Re: [PATCH 4/9] commit-reach: use `size_t` to track indices in `remove_redundant()`
- From
Justin Tobler <jltobler@gmail.com>
- Date
- Jan 3, 2025, 01:46 UTC
- Message-ID
- <miadr6xzejrkauyoqbj2su6drnz53xd443ylvxertawa7ovuke@42bigdksj5iw>
- In-Reply-To
- <20241227-b4-pks-commit-reach-sign-compare-v1-4-07c59c2aa632@pks.im>
On 24/12/27 11:46AM, Patrick Steinhardt wrote:
Show 51 quoted lines
> 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