{"thread":{"id":"63086","subject":"[GSoC PATCH] decorate: fix sign comparison warnings","startedAt":"2025-03-10T09:51:08Z","lastAt":"2025-03-11T18:26:45Z","messageCount":6,"participants":["Arnav Bhate","Junio C Hamano","Karthik Nayak"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"513890","messageId":"afa6b428-3190-42ae-9eac-540c95b576fd@gmail.com","threadId":"63086","inReplyTo":null,"subject":"[GSoC PATCH] decorate: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-10T09:51:11Z","receivedAt":"2025-03-10T09:51:08Z","isPatch":true,"sender":{"key":"bhatearnav@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86760472?v=4"},"body":"In two instances, an int was initialized and assigned the value of an\nunsigned int. Then, the int was compared to unsigned ints.\n\nReplace int with unsigned int in both cases.\n\nSigned-off-by: Arnav Bhate <bhatearnav@gmail.com>\n---\n decorate.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/decorate.c b/decorate.c\nindex e161e13772..8d5774fcdd 100644\n--- a/decorate.c\n+++ b/decorate.c\n@@ -3,8 +3,6 @@\n  * data.\n  */\n \n-#define DISABLE_SIGN_COMPARE_WARNINGS\n-\n #include \"git-compat-util.h\"\n #include \"object.h\"\n #include \"decorate.h\"\n@@ -16,7 +14,7 @@ static unsigned int hash_obj(const struct object *obj, unsigned int n)\n \n static void *insert_decoration(struct decoration *n, const struct object *base, void *decoration)\n {\n-\tint size = n->size;\n+\tunsigned int size = n->size;\n \tstruct decoration_entry *entries = n->entries;\n \tunsigned int j = hash_obj(base, size);\n \n@@ -59,7 +57,7 @@ static void grow_decoration(struct decoration *n)\n void *add_decoration(struct decoration *n, const struct object *obj,\n \t\tvoid *decoration)\n {\n-\tint nr = n->nr + 1;\n+\tunsigned int nr = n->nr + 1;\n \n \tif (nr > n->size * 2 / 3)\n \t\tgrow_decoration(n);\n-- \n2.48.1\n"},{"id":"513922","messageId":"xmqqikog96uz.fsf@gitster.g","threadId":"63086","inReplyTo":"afa6b428-3190-42ae-9eac-540c95b576fd@gmail.com","subject":"Re: [GSoC PATCH] decorate: fix sign comparison warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-10T16:40:36Z","receivedAt":"2025-03-10T16:40:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Arnav Bhate <bhatearnav@gmail.com> writes:\n\n> In two instances, an int was initialized and assigned the value of an\n> unsigned int. Then, the int was compared to unsigned ints.\n>\n> Replace int with unsigned int in both cases.\n\nAnd these places do not use a negative value to mean anything\nspecial.\n\nA simpler fix to the first hunk may be to get rid of the\nintermediate variable altogether and always refer to n->size\nwhen its value is needed.  The compiler should be able to see in\nthis static file-scope helper function that n->size would not change\nat all and do the right thing (i.e. allocate a register to hold its\nvalue at entry, if needed) without a hand-optimization we see in the\noriginal code.\n\nThe same can be said for the second hunk.  The intermediate variable\nis used only once, and one could argue that its presense obscures\nthe condition under which grow_decoration() is called by splitting a\nlogically single expression into two.\n\nWhich one is easier to grok?\n\n\tunsigned nr = n->nr + 1;\n\tif (nr > n->size * 2 / 3)\n\t\tgrow_decoration(n);\n\nor\n\t\n\tif ((n->nr + 1) > n->size * 2 / 3)\n\t\tgrow_decoration(n);\n\n\n> Signed-off-by: Arnav Bhate <bhatearnav@gmail.com>\n> ---\n>  decorate.c | 6 ++----\n>  1 file changed, 2 insertions(+), 4 deletions(-)\n>\n> diff --git a/decorate.c b/decorate.c\n> index e161e13772..8d5774fcdd 100644\n> --- a/decorate.c\n> +++ b/decorate.c\n> @@ -3,8 +3,6 @@\n>   * data.\n>   */\n>  \n> -#define DISABLE_SIGN_COMPARE_WARNINGS\n> -\n>  #include \"git-compat-util.h\"\n>  #include \"object.h\"\n>  #include \"decorate.h\"\n> @@ -16,7 +14,7 @@ static unsigned int hash_obj(const struct object *obj, unsigned int n)\n>  \n>  static void *insert_decoration(struct decoration *n, const struct object *base, void *decoration)\n>  {\n> -\tint size = n->size;\n> +\tunsigned int size = n->size;\n>  \tstruct decoration_entry *entries = n->entries;\n>  \tunsigned int j = hash_obj(base, size);\n>  \n> @@ -59,7 +57,7 @@ static void grow_decoration(struct decoration *n)\n>  void *add_decoration(struct decoration *n, const struct object *obj,\n>  \t\tvoid *decoration)\n>  {\n> -\tint nr = n->nr + 1;\n> +\tunsigned int nr = n->nr + 1;\n>  \n>  \tif (nr > n->size * 2 / 3)\n>  \t\tgrow_decoration(n);\n"},{"id":"513927","messageId":"d68646b7-0513-4a54-a145-cdbd0cd67a4a@gmail.com","threadId":"63086","inReplyTo":"xmqqikog96uz.fsf@gitster.g","subject":"Re: [GSoC PATCH] decorate: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-10T18:07:07Z","receivedAt":"2025-03-10T18:07:05Z","isPatch":true,"sender":{"key":"bhatearnav@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86760472?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> A simpler fix to the first hunk may be to get rid of the\n> intermediate variable altogether and always refer to n->size\n> when its value is needed.  The compiler should be able to see in\n> this static file-scope helper function that n->size would not change\n> at all and do the right thing (i.e. allocate a register to hold its\n> value at entry, if needed) without a hand-optimization we see in the\n> original code.\n> \n> The same can be said for the second hunk.  The intermediate variable\n> is used only once, and one could argue that its presense obscures\n> the condition under which grow_decoration() is called by splitting a\n> logically single expression into two.\n> \n> Which one is easier to grok?\n> \n> \tunsigned nr = n->nr + 1;\n> \tif (nr > n->size * 2 / 3)\n> \t\tgrow_decoration(n);\n> \n> or\n> \t\n> \tif ((n->nr + 1) > n->size * 2 / 3)\n> \t\tgrow_decoration(n);\n\nYour suggestion makes sense to me, the second one is better. I will send an\nupdated patch. I also found some more places in the file where a change from\nint to unsigned int should happen, but where int does not cause warnings. I\nwill also include it in the patch.\n\n-- \nRegards,\nArnav Bhate\n(He/Him)\n\n"},{"id":"513928","messageId":"7c219279-8151-49c0-8fc0-8abe2624aca9@gmail.com","threadId":"63086","inReplyTo":"afa6b428-3190-42ae-9eac-540c95b576fd@gmail.com","subject":"[GSoC PATCH v2] decorate: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-10T18:08:53Z","receivedAt":"2025-03-10T18:08:50Z","isPatch":true,"sender":{"key":"bhatearnav@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86760472?v=4"},"body":"There are multiple instances where ints have been initialized with\nvalues of unsigned ints, and where negative values don't mean anything.\nWhen such ints are compared with unsigned ints, it causes sign comparison\nwarnings.\n\nAlso, some of these are used just as stand-ins for their initial\nvalues, never being modified, thus obscuring the specific conditions\nunder which certain operations happen.\n\nReplace int with unsigned int for 2 variables, and replace the\nintermediate variables with their initial values for 2 other variables.\n\nSigned-off-by: Arnav Bhate <bhatearnav@gmail.com>\n---\n decorate.c | 15 +++++----------\n 1 file changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/decorate.c b/decorate.c\nindex e161e13772..9f24925263 100644\n--- a/decorate.c\n+++ b/decorate.c\n@@ -3,8 +3,6 @@\n  * data.\n  */\n \n-#define DISABLE_SIGN_COMPARE_WARNINGS\n-\n #include \"git-compat-util.h\"\n #include \"object.h\"\n #include \"decorate.h\"\n@@ -16,9 +14,8 @@ static unsigned int hash_obj(const struct object *obj, unsigned int n)\n \n static void *insert_decoration(struct decoration *n, const struct object *base, void *decoration)\n {\n-\tint size = n->size;\n \tstruct decoration_entry *entries = n->entries;\n-\tunsigned int j = hash_obj(base, size);\n+\tunsigned int j = hash_obj(base, n->size);\n \n \twhile (entries[j].base) {\n \t\tif (entries[j].base == base) {\n@@ -26,7 +23,7 @@ static void *insert_decoration(struct decoration *n, const struct object *base,\n \t\t\tentries[j].decoration = decoration;\n \t\t\treturn old;\n \t\t}\n-\t\tif (++j >= size)\n+\t\tif (++j >= n->size)\n \t\t\tj = 0;\n \t}\n \tentries[j].base = base;\n@@ -37,8 +34,8 @@ static void *insert_decoration(struct decoration *n, const struct object *base,\n \n static void grow_decoration(struct decoration *n)\n {\n-\tint i;\n-\tint old_size = n->size;\n+\tunsigned int i;\n+\tunsigned int old_size = n->size;\n \tstruct decoration_entry *old_entries = n->entries;\n \n \tn->size = (old_size + 1000) * 3 / 2;\n@@ -59,9 +56,7 @@ static void grow_decoration(struct decoration *n)\n void *add_decoration(struct decoration *n, const struct object *obj,\n \t\tvoid *decoration)\n {\n-\tint nr = n->nr + 1;\n-\n-\tif (nr > n->size * 2 / 3)\n+\tif ((n->nr + 1) > n->size * 2 / 3)\n \t\tgrow_decoration(n);\n \treturn insert_decoration(n, obj, decoration);\n }\n-- \n2.48.1\n"},{"id":"513942","messageId":"CAOLa=ZS=+UGf5yQO3nVhLFJ7BgsSSxigKA3CTqEVez2zkQKyoA@mail.gmail.com","threadId":"63086","inReplyTo":"7c219279-8151-49c0-8fc0-8abe2624aca9@gmail.com","subject":"Re: [GSoC PATCH v2] decorate: fix sign comparison warnings","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-03-10T21:02:32Z","receivedAt":"2025-03-10T21:02:36Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Arnav Bhate <bhatearnav@gmail.com> writes:\n\n> There are multiple instances where ints have been initialized with\n> values of unsigned ints, and where negative values don't mean anything.\n> When such ints are compared with unsigned ints, it causes sign comparison\n> warnings.\n>\n> Also, some of these are used just as stand-ins for their initial\n> values, never being modified, thus obscuring the specific conditions\n> under which certain operations happen.\n>\n> Replace int with unsigned int for 2 variables, and replace the\n> intermediate variables with their initial values for 2 other variables.\n\nNit: worthwhile to mention that we also remove the\n`DISABLE_SIGN_COMPARE_WARNINGS` macro as a result of this change.\n\n>\n> Signed-off-by: Arnav Bhate <bhatearnav@gmail.com>\n> ---\n>  decorate.c | 15 +++++----------\n>  1 file changed, 5 insertions(+), 10 deletions(-)\n>\n> diff --git a/decorate.c b/decorate.c\n> index e161e13772..9f24925263 100644\n> --- a/decorate.c\n> +++ b/decorate.c\n> @@ -3,8 +3,6 @@\n>   * data.\n>   */\n>\n> -#define DISABLE_SIGN_COMPARE_WARNINGS\n> -\n>  #include \"git-compat-util.h\"\n>  #include \"object.h\"\n>  #include \"decorate.h\"\n> @@ -16,9 +14,8 @@ static unsigned int hash_obj(const struct object *obj, unsigned int n)\n>\n>  static void *insert_decoration(struct decoration *n, const struct object *base, void *decoration)\n>  {\n> -\tint size = n->size;\n>  \tstruct decoration_entry *entries = n->entries;\n> -\tunsigned int j = hash_obj(base, size);\n> +\tunsigned int j = hash_obj(base, n->size);\n>\n>  \twhile (entries[j].base) {\n>  \t\tif (entries[j].base == base) {\n> @@ -26,7 +23,7 @@ static void *insert_decoration(struct decoration *n, const struct object *base,\n>  \t\t\tentries[j].decoration = decoration;\n>  \t\t\treturn old;\n>  \t\t}\n> -\t\tif (++j >= size)\n> +\t\tif (++j >= n->size)\n>  \t\t\tj = 0;\n>  \t}\n>  \tentries[j].base = base;\n> @@ -37,8 +34,8 @@ static void *insert_decoration(struct decoration *n, const struct object *base,\n>\n>  static void grow_decoration(struct decoration *n)\n>  {\n> -\tint i;\n> -\tint old_size = n->size;\n> +\tunsigned int i;\n> +\tunsigned int old_size = n->size;\n>  \tstruct decoration_entry *old_entries = n->entries;\n>\n\nI was wondering why we don't use `n->size` like the previous hunk. Seems\nlike its because `n->size` is modified right after.\n\nLooking into the code, perhaps this code could be moved to using\nALLOW_GROW. But that is totally outside this patch.\n\n>  \tn->size = (old_size + 1000) * 3 / 2;\n> @@ -59,9 +56,7 @@ static void grow_decoration(struct decoration *n)\n>  void *add_decoration(struct decoration *n, const struct object *obj,\n>  \t\tvoid *decoration)\n>  {\n> -\tint nr = n->nr + 1;\n> -\n> -\tif (nr > n->size * 2 / 3)\n> +\tif ((n->nr + 1) > n->size * 2 / 3)\n>  \t\tgrow_decoration(n);\n>  \treturn insert_decoration(n, obj, decoration);\n>  }\n> --\n> 2.48.1\n\nThe patch looks good!\n\nThanks\n"},{"id":"513997","messageId":"c7045e4a-51af-4fff-b6e3-36afff005c77@gmail.com","threadId":"63086","inReplyTo":"CAOLa=ZS=+UGf5yQO3nVhLFJ7BgsSSxigKA3CTqEVez2zkQKyoA@mail.gmail.com","subject":"Re: [GSoC PATCH v2] decorate: fix sign comparison warnings","fromName":"Arnav Bhate","fromEmail":"bhatearnav@gmail.com","sentAt":"2025-03-11T18:26:42Z","receivedAt":"2025-03-11T18:26:45Z","isPatch":true,"sender":{"key":"bhatearnav@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86760472?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n> Arnav Bhate <bhatearnav@gmail.com> writes:\n> \n>> There are multiple instances where ints have been initialized with\n>> values of unsigned ints, and where negative values don't mean anything.\n>> When such ints are compared with unsigned ints, it causes sign comparison\n>> warnings.\n>>\n>> Also, some of these are used just as stand-ins for their initial\n>> values, never being modified, thus obscuring the specific conditions\n>> under which certain operations happen.\n>>\n>> Replace int with unsigned int for 2 variables, and replace the\n>> intermediate variables with their initial values for 2 other variables.\n> \n> Nit: worthwhile to mention that we also remove the\n> `DISABLE_SIGN_COMPARE_WARNINGS` macro as a result of this change.\n\nI'll keep this in mind for my next contribution.\n\n-- \nRegards,\nArnav Bhate\n(He/Him)\n\n"}]}