[GSoC PATCH] decorate: fix sign comparison warnings

[GSoC PATCH] decorate: fix sign comparison warnings

[Arnav Bhate]

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(-)

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);
-- 
2.48.1

Re: [GSoC PATCH] decorate: fix sign comparison warnings

[Junio C Hamano]

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);

Re: [GSoC PATCH] decorate: fix sign comparison warnings

[Arnav Bhate]

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.

-- 
Regards,
Arnav Bhate
(He/Him)

[GSoC PATCH v2] decorate: fix sign comparison warnings

[Arnav Bhate]

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(-)

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);
 }
-- 
2.48.1

Re: [GSoC PATCH v2] decorate: fix sign comparison warnings

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2827 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [GSoC PATCH v2] decorate: fix sign comparison warnings

[Arnav Bhate]

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.

-- 
Regards,
Arnav Bhate
(He/Him)