| Message ID | afa6b428-3190-42ae-9eac-540c95b576fd@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [GSoC] decorate: fix sign comparison warnings | expand |
Arnav Bhate <bhatearnav@gmail.com> writes: > In two instances, an int was initialized and assigned the value of an > unsigned int. Then, the int was compared to unsigned ints. > > Replace int with unsigned int in both cases. And these places do not use a negative value to mean anything special. A simpler fix to the first hunk may be to get rid of the intermediate variable altogether and always refer to n->size when its value is needed. The compiler should be able to see in this static file-scope helper function that n->size would not change at all and do the right thing (i.e. allocate a register to hold its value at entry, if needed) without a hand-optimization we see in the original code. The same can be said for the second hunk. The intermediate variable is used only once, and one could argue that its presense obscures the condition under which grow_decoration() is called by splitting a logically single expression into two. Which one is easier to grok? unsigned nr = n->nr + 1; if (nr > n->size * 2 / 3) grow_decoration(n); or if ((n->nr + 1) > n->size * 2 / 3) grow_decoration(n); > Signed-off-by: Arnav Bhate <bhatearnav@gmail.com> > --- > decorate.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/decorate.c b/decorate.c > index e161e13772..8d5774fcdd 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,7 +14,7 @@ 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; > + unsigned int size = n->size; > struct decoration_entry *entries = n->entries; > unsigned int j = hash_obj(base, size); > > @@ -59,7 +57,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; > + unsigned int nr = n->nr + 1; > > if (nr > n->size * 2 / 3) > grow_decoration(n);
Junio C Hamano <gitster@pobox.com> writes: > A simpler fix to the first hunk may be to get rid of the > intermediate variable altogether and always refer to n->size > when its value is needed. The compiler should be able to see in > this static file-scope helper function that n->size would not change > at all and do the right thing (i.e. allocate a register to hold its > value at entry, if needed) without a hand-optimization we see in the > original code. > > The same can be said for the second hunk. The intermediate variable > is used only once, and one could argue that its presense obscures > the condition under which grow_decoration() is called by splitting a > logically single expression into two. > > Which one is easier to grok? > > unsigned nr = n->nr + 1; > if (nr > n->size * 2 / 3) > grow_decoration(n); > > or > > if ((n->nr + 1) > n->size * 2 / 3) > grow_decoration(n); Your suggestion makes sense to me, the second one is better. I will send an updated patch. I also found some more places in the file where a change from int to unsigned int should happen, but where int does not cause warnings. I will also include it in the patch.
diff --git a/decorate.c b/decorate.c index e161e13772..8d5774fcdd 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,7 +14,7 @@ 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; + unsigned int size = n->size; struct decoration_entry *entries = n->entries; unsigned int j = hash_obj(base, size); @@ -59,7 +57,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; + unsigned int nr = n->nr + 1; if (nr > n->size * 2 / 3) grow_decoration(n);
In two instances, an int was initialized and assigned the value of an unsigned int. Then, the int was compared to unsigned ints. Replace int with unsigned int in both cases. Signed-off-by: Arnav Bhate <bhatearnav@gmail.com> --- decorate.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)