Re: [GSoC PATCH v2] decorate: fix sign comparison warnings
Arnav Bhate <bhatearnav@gmail.com> writes:
Show 11 quoted lines
> There are multiple instances where ints have been initialized with
> values of unsigned ints, and where negative values don't mean anything.
> When such ints are compared with unsigned ints, it causes sign comparison
> warnings.
>
> Also, some of these are used just as stand-ins for their initial
> values, never being modified, thus obscuring the specific conditions
> under which certain operations happen.
>
> Replace int with unsigned int for 2 variables, and replace the
> intermediate variables with their initial values for 2 other variables.
Nit: worthwhile to mention that we also remove the
`DISABLE_SIGN_COMPARE_WARNINGS` macro as a result of this change.
Show 49 quoted lines
>
> Signed-off-by: Arnav Bhate <bhatearnav@gmail.com>
> ---
> decorate.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/decorate.c b/decorate.c
> index e161e13772..9f24925263 100644
> --- a/decorate.c
> +++ b/decorate.c
> @@ -3,8 +3,6 @@
> * data.
> */
>
> -#define DISABLE_SIGN_COMPARE_WARNINGS
> -
> #include "git-compat-util.h"
> #include "object.h"
> #include "decorate.h"
> @@ -16,9 +14,8 @@ static unsigned int hash_obj(const struct object *obj, unsigned int n)
>
> static void *insert_decoration(struct decoration *n, const struct object *base, void *decoration)
> {
> - int size = n->size;
> struct decoration_entry *entries = n->entries;
> - unsigned int j = hash_obj(base, size);
> + unsigned int j = hash_obj(base, n->size);
>
> while (entries[j].base) {
> if (entries[j].base == base) {
> @@ -26,7 +23,7 @@ static void *insert_decoration(struct decoration *n, const struct object *base,
> entries[j].decoration = decoration;
> return old;
> }
> - if (++j >= size)
> + if (++j >= n->size)
> j = 0;
> }
> entries[j].base = base;
> @@ -37,8 +34,8 @@ static void *insert_decoration(struct decoration *n, const struct object *base,
>
> static void grow_decoration(struct decoration *n)
> {
> - int i;
> - int old_size = n->size;
> + unsigned int i;
> + unsigned int old_size = n->size;
> struct decoration_entry *old_entries = n->entries;
>I was wondering why we don't use `n->size` like the previous hunk. Seems like its because `n->size` is modified right after.
Looking into the code, perhaps this code could be moved to using ALLOW_GROW. But that is totally outside this patch.
Show 14 quoted lines
> n->size = (old_size + 1000) * 3 / 2;
> @@ -59,9 +56,7 @@ static void grow_decoration(struct decoration *n)
> void *add_decoration(struct decoration *n, const struct object *obj,
> void *decoration)
> {
> - int nr = n->nr + 1;
> -
> - if (nr > n->size * 2 / 3)
> + if ((n->nr + 1) > n->size * 2 / 3)
> grow_decoration(n);
> return insert_decoration(n, obj, decoration);
> }
> --
> 2.48.1The patch looks good!
Thanks
Arnav Bhate
