| Message ID | 20241227-b4-pks-commit-reach-sign-compare-v1-7-07c59c2aa632@pks.im (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | commit-reach: -Wsign-compare follow-ups | expand |
On Fri, Dec 27, 2024 at 11:46:27AM +0100, Patrick Steinhardt wrote: [snip] > @@ -717,14 +715,14 @@ static int show_tag_object(const struct object_id *oid, struct rev_info *rev) > unsigned long size; > enum object_type type; > char *buf = repo_read_object_file(the_repository, oid, &type, &size); > - int offset = 0; > + unsigned long offset = 0; Why here we use `unsigned long`, is this a special situation where we cannot use `size_t`? > > if (!buf) > return error(_("could not read object %s"), oid_to_hex(oid)); > > assert(type == OBJ_TAG); > while (offset < size && buf[offset] != '\n') { > - int new_offset = offset + 1; > + unsigned long new_offset = offset + 1; > const char *ident; > while (new_offset < size && buf[new_offset++] != '\n') > ; /* do nothing */ > @@ -2183,7 +2182,7 @@ int cmd_format_patch(int argc, > fmt_patch_suffix = cfg.fmt_patch_suffix; > > /* Make sure "0000-$sub.patch" gives non-negative length for $sub */ > - if (cfg.log.fmt_patch_name_max <= strlen("0000-") + strlen(fmt_patch_suffix)) > + if (cfg.log.fmt_patch_name_max <= cast_size_t_to_int(strlen("0000-") + strlen(fmt_patch_suffix))) A design question, why we don't change the type of `cfg.log.fmt_patch_name_max` to be `size_t`? > cfg.log.fmt_patch_name_max = strlen("0000-") + strlen(fmt_patch_suffix); > > if (cover_from_description_arg) > > -- > 2.48.0.rc0.184.g0fc57dec57.dirty > Thanks, Jialuo
On Fri, Dec 27, 2024 at 09:21:56PM +0800, shejialuo wrote: > On Fri, Dec 27, 2024 at 11:46:27AM +0100, Patrick Steinhardt wrote: > > @@ -717,14 +715,14 @@ static int show_tag_object(const struct object_id *oid, struct rev_info *rev) > > unsigned long size; > > enum object_type type; > > char *buf = repo_read_object_file(the_repository, oid, &type, &size); > > - int offset = 0; > > + unsigned long offset = 0; > > Why here we use `unsigned long`, is this a special situation where we > cannot use `size_t`? Mostly because other variables already use `unsigned long` here, including `repo_read_object_file()`. So given that our object layer doesn't support `size_t` it wouldn't make sense to use it for the offset, either. > > > > if (!buf) > > return error(_("could not read object %s"), oid_to_hex(oid)); > > > > assert(type == OBJ_TAG); > > while (offset < size && buf[offset] != '\n') { > > - int new_offset = offset + 1; > > + unsigned long new_offset = offset + 1; > > const char *ident; > > while (new_offset < size && buf[new_offset++] != '\n') > > ; /* do nothing */ > > > @@ -2183,7 +2182,7 @@ int cmd_format_patch(int argc, > > fmt_patch_suffix = cfg.fmt_patch_suffix; > > > > /* Make sure "0000-$sub.patch" gives non-negative length for $sub */ > > - if (cfg.log.fmt_patch_name_max <= strlen("0000-") + strlen(fmt_patch_suffix)) > > + if (cfg.log.fmt_patch_name_max <= cast_size_t_to_int(strlen("0000-") + strlen(fmt_patch_suffix))) > > A design question, why we don't change the type of > `cfg.log.fmt_patch_name_max` to be `size_t`? The whole infra around this uses `int`s, too, so changing this variable here alone wouldn't suffice. We'd also have to adapt option handling, config handling, `struct rev_info::patch_name_max` and other bits and pieces. So in the end the size of required changes would likely balloon and thus I decided to not go down this rabbit hole. Patrick
On Fri, Dec 27, 2024 at 02:57:18PM +0100, Patrick Steinhardt wrote: > On Fri, Dec 27, 2024 at 09:21:56PM +0800, shejialuo wrote: > > On Fri, Dec 27, 2024 at 11:46:27AM +0100, Patrick Steinhardt wrote: > > > @@ -717,14 +715,14 @@ static int show_tag_object(const struct object_id *oid, struct rev_info *rev) > > > unsigned long size; > > > enum object_type type; > > > char *buf = repo_read_object_file(the_repository, oid, &type, &size); > > > - int offset = 0; > > > + unsigned long offset = 0; > > > > Why here we use `unsigned long`, is this a special situation where we > > cannot use `size_t`? > > Mostly because other variables already use `unsigned long` here, > including `repo_read_object_file()`. So given that our object layer > doesn't support `size_t` it wouldn't make sense to use it for the > offset, either. > Make sense. Thanks for the explanation. > > > > > > if (!buf) > > > return error(_("could not read object %s"), oid_to_hex(oid)); > > > > > > assert(type == OBJ_TAG); > > > while (offset < size && buf[offset] != '\n') { > > > - int new_offset = offset + 1; > > > + unsigned long new_offset = offset + 1; > > > const char *ident; > > > while (new_offset < size && buf[new_offset++] != '\n') > > > ; /* do nothing */ > > > > > @@ -2183,7 +2182,7 @@ int cmd_format_patch(int argc, > > > fmt_patch_suffix = cfg.fmt_patch_suffix; > > > > > > /* Make sure "0000-$sub.patch" gives non-negative length for $sub */ > > > - if (cfg.log.fmt_patch_name_max <= strlen("0000-") + strlen(fmt_patch_suffix)) > > > + if (cfg.log.fmt_patch_name_max <= cast_size_t_to_int(strlen("0000-") + strlen(fmt_patch_suffix))) > > > > A design question, why we don't change the type of > > `cfg.log.fmt_patch_name_max` to be `size_t`? > > The whole infra around this uses `int`s, too, so changing this variable > here alone wouldn't suffice. We'd also have to adapt option handling, > config handling, `struct rev_info::patch_name_max` and other bits and > pieces. So in the end the size of required changes would likely balloon > and thus I decided to not go down this rabbit hole. > Yes, I agree. If we do this, there is a lot of efforts we need to deal with. > Patrick
diff --git a/builtin/log.c b/builtin/log.c index 805b2355d964915732edf5928d54fb6d06e394d4..a4f41aafcae069541ee987dc94d245edfe9787a8 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -6,7 +6,6 @@ */ #define USE_THE_REPOSITORY_VARIABLE -#define DISABLE_SIGN_COMPARE_WARNINGS #include "builtin.h" #include "abspath.h" @@ -209,7 +208,6 @@ static void cmd_log_init_defaults(struct rev_info *rev, static void set_default_decoration_filter(struct decoration_filter *decoration_filter) { - int i; char *value = NULL; struct string_list *include = decoration_filter->include_ref_pattern; const struct string_list *config_exclude; @@ -243,7 +241,7 @@ static void set_default_decoration_filter(struct decoration_filter *decoration_f * No command-line or config options were given, so * populate with sensible defaults. */ - for (i = 0; i < ARRAY_SIZE(ref_namespace); i++) { + for (size_t i = 0; i < ARRAY_SIZE(ref_namespace); i++) { if (!ref_namespace[i].decoration) continue; @@ -717,14 +715,14 @@ static int show_tag_object(const struct object_id *oid, struct rev_info *rev) unsigned long size; enum object_type type; char *buf = repo_read_object_file(the_repository, oid, &type, &size); - int offset = 0; + unsigned long offset = 0; if (!buf) return error(_("could not read object %s"), oid_to_hex(oid)); assert(type == OBJ_TAG); while (offset < size && buf[offset] != '\n') { - int new_offset = offset + 1; + unsigned long new_offset = offset + 1; const char *ident; while (new_offset < size && buf[new_offset++] != '\n') ; /* do nothing */ @@ -1316,24 +1314,25 @@ static void print_signature(const char *signature, FILE *file) static char *find_branch_name(struct rev_info *rev) { - int i, positive = -1; struct object_id branch_oid; const struct object_id *tip_oid; const char *ref, *v; char *full_ref, *branch = NULL; + int interesting_found = 0; + size_t idx; - for (i = 0; i < rev->cmdline.nr; i++) { + for (size_t i = 0; i < rev->cmdline.nr; i++) { if (rev->cmdline.rev[i].flags & UNINTERESTING) continue; - if (positive < 0) - positive = i; - else + if (interesting_found) return NULL; + interesting_found = 1; + idx = i; } - if (positive < 0) + if (!interesting_found) return NULL; - ref = rev->cmdline.rev[positive].name; - tip_oid = &rev->cmdline.rev[positive].item->oid; + ref = rev->cmdline.rev[idx].name; + tip_oid = &rev->cmdline.rev[idx].item->oid; if (repo_dwim_ref(the_repository, ref, strlen(ref), &branch_oid, &full_ref, 0) && skip_prefix(full_ref, "refs/heads/", &v) && @@ -2183,7 +2182,7 @@ int cmd_format_patch(int argc, fmt_patch_suffix = cfg.fmt_patch_suffix; /* Make sure "0000-$sub.patch" gives non-negative length for $sub */ - if (cfg.log.fmt_patch_name_max <= strlen("0000-") + strlen(fmt_patch_suffix)) + if (cfg.log.fmt_patch_name_max <= cast_size_t_to_int(strlen("0000-") + strlen(fmt_patch_suffix))) cfg.log.fmt_patch_name_max = strlen("0000-") + strlen(fmt_patch_suffix); if (cover_from_description_arg)
Fix remaining -Wsign-compare warnings in "builtin/log.c" and mark the file as -Wsign-compare-clean. While most of the fixes are obvious, one fix requires us to use `cast_size_t_to_int()`, which will cause us to die in case the `size_t` cannot be represented as `int`. This should be fine though, as the data would typically be set either via a config key or via the command line, neither of which should ever exceed a couple of kilobytes of data. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/log.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)