Re: [PATCH v2 01/13] reftable/block: use `size_t` to track restart point index

From: Karthik Nayak <karthik.188@gmail.com>
Date: May 21, 2024, 13:34 UTC
Message-ID: <CAOLa=ZQb3if7vcvRcOm8qHSMGo=dSv0nsjCk0R_hhRWU_d7wfw@mail.gmail.com>
In-Reply-To: <ca86a8b58dd074287dd2dd352610ffe46e1605b9.1715589670.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 6 quoted lines

> The function `block_reader_restart_offset()` gets the offset of the
> `i`th restart point. `i` is a signed integer though, which is certainly
> not the correct type to track indices like this. Furthermore, both
> callers end up passing a `size_t`.
>
> Refactor the code to use a `size_t` instead.

More of a question for my understanding: Why use `size_t` vs `uint16_t` here? I'm asking since the restart count is defined as `uint16_t restart_count` in `struct block_reader`.

Show 24 quoted lines

>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  reftable/block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/reftable/block.c b/reftable/block.c
> index 5942cb4053..00030eee06 100644
> --- a/reftable/block.c
> +++ b/reftable/block.c
> @@ -326,9 +326,9 @@ int block_reader_first_key(const struct block_reader *br, struct strbuf *key)
>  	return 0;
>  }
>
> -static uint32_t block_reader_restart_offset(const struct block_reader *br, int i)
> +static uint32_t block_reader_restart_offset(const struct block_reader *br, size_t idx)
>  {
> -	return get_be24(br->restart_bytes + 3 * i);
> +	return get_be24(br->restart_bytes + 3 * idx);
>  }
>
>  void block_iter_seek_start(struct block_iter *it, const struct block_reader *br)
> --
> 2.45.GIT