| Message ID | 20250116-b4-pks-reftable-sign-compare-v1-10-bd30e2ee96e7@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reftable: fix -Wsign-compare warnings | expand |
Patrick Steinhardt <ps@pks.im> writes: > Address the last couple of trivial -Wsign-compare warnings in the > reftable library and remove the DISABLE_SIGN_COMPARE_WARNINGS macro that > we have in "reftable/system.h". Hmph. > - int l = strlen(str); > + size_t l = strlen(str); Obviously correct. > - j = 1; > - while (j < count) { > + for (uint64_t j = 1; j < count; j++) { OK. As count is u64, it makes sense to match. > diff --git a/reftable/stack.c b/reftable/stack.c > index 531660a49f..5c0d6273a7 100644 > --- a/reftable/stack.c > +++ b/reftable/stack.c > @@ -220,9 +220,9 @@ void reftable_stack_destroy(struct reftable_stack *st) > } > > if (st->readers) { > - int i = 0; > struct reftable_buf filename = REFTABLE_BUF_INIT; > - for (i = 0; i < st->readers_len; i++) { > + > + for (size_t i = 0; i < st->readers_len; i++) { Likewise, readers_len is size_t so counters can match it. > - for (i = 0; i < st->readers_len; i++) { > + for (size_t i = 0; i < st->readers_len; i++) { Ditto. > - for (i = 0; !found && i < st->readers_len; i++) { > + for (size_t i = 0; !found && i < st->readers_len; i++) Ditto. > diff --git a/reftable/writer.c b/reftable/writer.c > index 4e6ca2e368..91d6629486 100644 > --- a/reftable/writer.c > +++ b/reftable/writer.c > @@ -577,7 +577,7 @@ static int writer_finish_section(struct reftable_writer *w) > > struct common_prefix_arg { > struct reftable_buf *last; > - int max; > + size_t max; > }; This is dubious. write.c:update_common() uses this to keep track of the maximum value returned by common_prefix_size(), which returns an int. writer.c:writer_dump_object_index() assigns the value comparable to this member to reftable_stats.object_id_len that is of type int. I may be more sympathetic if we were making common_prefix_size() return size_t instead of "int" and dealing with all the fallouts from such a change, but this smells more like somebody _else_ is using size_t on something that is not an allocation size, and such mixed use is cascading down to contaminate this member, which would be perfectly fine with platform native "int". Ah, OK, an earlier patch does change these other things to size_t, so this must change to size_t to be consistent. Shouldn't it be done in the same patch, then, though?
On Thu, Jan 16, 2025 at 02:12:33PM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > diff --git a/reftable/writer.c b/reftable/writer.c > > index 4e6ca2e368..91d6629486 100644 > > --- a/reftable/writer.c > > +++ b/reftable/writer.c > > @@ -577,7 +577,7 @@ static int writer_finish_section(struct reftable_writer *w) > > > > struct common_prefix_arg { > > struct reftable_buf *last; > > - int max; > > + size_t max; > > }; > > This is dubious. write.c:update_common() uses this to keep track of > the maximum value returned by common_prefix_size(), which returns > an int. writer.c:writer_dump_object_index() assigns the value > comparable to this member to reftable_stats.object_id_len that is of > type int. > > I may be more sympathetic if we were making common_prefix_size() > return size_t instead of "int" and dealing with all the fallouts > from such a change, but this smells more like somebody _else_ is > using size_t on something that is not an allocation size, and such > mixed use is cascading down to contaminate this member, which would > be perfectly fine with platform native "int". > > Ah, OK, an earlier patch does change these other things to size_t, > so this must change to size_t to be consistent. Shouldn't it be > done in the same patch, then, though? Good point indeed. I'll move the change around, thanks! Patrick
diff --git a/reftable/record.c b/reftable/record.c index 0ce294078b..cff7564087 100644 --- a/reftable/record.c +++ b/reftable/record.c @@ -117,7 +117,7 @@ static int decode_string(struct reftable_buf *dest, struct string_view in) static int encode_string(const char *str, struct string_view s) { struct string_view start = s; - int l = strlen(str); + size_t l = strlen(str); int n = put_var_int(&s, l); if (n < 0) return -1; @@ -556,7 +556,6 @@ static int reftable_obj_record_decode(void *rec, struct reftable_buf key, uint64_t count = val_type; int n = 0; uint64_t last; - int j; reftable_obj_record_release(r); @@ -591,8 +590,7 @@ static int reftable_obj_record_decode(void *rec, struct reftable_buf key, string_view_consume(&in, n); last = r->offsets[0]; - j = 1; - while (j < count) { + for (uint64_t j = 1; j < count; j++) { uint64_t delta = 0; int n = get_var_int(&delta, &in); if (n < 0) { @@ -601,7 +599,6 @@ static int reftable_obj_record_decode(void *rec, struct reftable_buf key, string_view_consume(&in, n); last = r->offsets[j] = (delta + last); - j++; } return start.len - in.len; } diff --git a/reftable/stack.c b/reftable/stack.c index 531660a49f..5c0d6273a7 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -220,9 +220,9 @@ void reftable_stack_destroy(struct reftable_stack *st) } if (st->readers) { - int i = 0; struct reftable_buf filename = REFTABLE_BUF_INIT; - for (i = 0; i < st->readers_len; i++) { + + for (size_t i = 0; i < st->readers_len; i++) { const char *name = reader_name(st->readers[i]); int try_unlinking = 1; @@ -238,6 +238,7 @@ void reftable_stack_destroy(struct reftable_stack *st) unlink(filename.buf); } } + reftable_buf_release(&filename); st->readers_len = 0; REFTABLE_FREE_AND_NULL(st->readers); @@ -568,7 +569,6 @@ static int stack_uptodate(struct reftable_stack *st) { char **names = NULL; int err; - int i = 0; /* * When we have cached stat information available then we use it to @@ -608,7 +608,7 @@ static int stack_uptodate(struct reftable_stack *st) if (err < 0) return err; - for (i = 0; i < st->readers_len; i++) { + for (size_t i = 0; i < st->readers_len; i++) { if (!names[i]) { err = 1; goto done; @@ -1767,14 +1767,12 @@ static int reftable_stack_clean_locked(struct reftable_stack *st) } while ((d = readdir(dir))) { - int i = 0; int found = 0; if (!is_table_name(d->d_name)) continue; - for (i = 0; !found && i < st->readers_len; i++) { + for (size_t i = 0; !found && i < st->readers_len; i++) found = !strcmp(reader_name(st->readers[i]), d->d_name); - } if (found) continue; diff --git a/reftable/system.h b/reftable/system.h index 5274eca1d0..7d5f803eeb 100644 --- a/reftable/system.h +++ b/reftable/system.h @@ -11,8 +11,6 @@ license that can be found in the LICENSE file or at /* This header glues the reftable library to the rest of Git */ -#define DISABLE_SIGN_COMPARE_WARNINGS - #include "git-compat-util.h" /* diff --git a/reftable/writer.c b/reftable/writer.c index 4e6ca2e368..91d6629486 100644 --- a/reftable/writer.c +++ b/reftable/writer.c @@ -577,7 +577,7 @@ static int writer_finish_section(struct reftable_writer *w) struct common_prefix_arg { struct reftable_buf *last; - int max; + size_t max; }; static void update_common(void *void_arg, void *key)
Address the last couple of trivial -Wsign-compare warnings in the reftable library and remove the DISABLE_SIGN_COMPARE_WARNINGS macro that we have in "reftable/system.h". Signed-off-by: Patrick Steinhardt <ps@pks.im> --- reftable/record.c | 7 ++----- reftable/stack.c | 12 +++++------- reftable/system.h | 2 -- reftable/writer.c | 2 +- 4 files changed, 8 insertions(+), 15 deletions(-)