| Message ID | 7c219279-8151-49c0-8fc0-8abe2624aca9@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [GSoC,v2] decorate: fix sign comparison warnings | expand |
Arnav Bhate <bhatearnav@gmail.com> writes: > 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. > > 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. > 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.1 The patch looks good! Thanks
Karthik Nayak <karthik.188@gmail.com> writes: > Arnav Bhate <bhatearnav@gmail.com> writes: > >> 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. I'll keep this in mind for my next contribution.
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; 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); }
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. Signed-off-by: Arnav Bhate <bhatearnav@gmail.com> --- decorate.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)