| Message ID | ca86a8b58dd074287dd2dd352610ffe46e1605b9.1715589670.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d537ce6b9ed75d50c6b8ad071439b06c1b70c5f8 |
| Headers | show |
| Series | reftable: prepare for re-seekable iterators | expand |
Patrick Steinhardt <ps@pks.im> writes: > 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`. > > 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
On Tue, May 21, 2024 at 01:34:48PM +0000, Karthik Nayak wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > 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`. Mostly because all callers already use `size_t`, and it's customary to use them when talking about indices. We could use `uint16_t`, too, but it didn't really feel worth it to adjust all callers. Patrick
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)
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. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- reftable/block.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)