{"thread":{"id":"55812","subject":"[PATCH] builtin/fsck.c: don't conflate \"int\" and \"enum\" in callback","startedAt":"2021-06-01T00:06:14Z","lastAt":"2021-06-01T21:04:38Z","messageCount":5,"participants":["Ævar Arnfjörð Bjarmason","Johannes Sixt","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"426085","messageId":"patch-1.1-f109a61b11-20210601T000433Z-avarab@gmail.com","threadId":"55812","inReplyTo":null,"subject":"[PATCH] builtin/fsck.c: don't conflate \"int\" and \"enum\" in callback","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-01T00:05:59Z","receivedAt":"2021-06-01T00:06:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a warning on AIX's xlc compiler that's been emitted since my\na1aad71601a (fsck.h: use \"enum object_type\" instead of \"int\",\n2021-03-28):\n\n    \"builtin/fsck.c\", line 805.32: 1506-068 (W) Operation between\n    types \"int(*)(struct object*,enum object_type,void*,struct\n    fsck_options*)\" and \"int(*)(struct object*,int,void*,struct\n    fsck_options*)\" is not allowed.\n\nI.e. it complains about us assigning a function with a prototype \"int\"\nwhere we're expecting \"enum object_type\". Enums are just ints in C,\nbut it seems xlc is more picky than some about conflating them at the\nsource level.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nThis is new in v2.32.0, so sending this during the rc phase, just a\nwarning though, so unless you're using fatal warnings on that\nOS/platform it won't impact anything, and even then it's just a minor\nannoyance...\n\n builtin/fsck.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 87a99b0108..b42b6fe21f 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -109,7 +109,8 @@ static int fsck_error_func(struct fsck_options *o,\n \n static struct object_array pending;\n \n-static int mark_object(struct object *obj, int type, void *data, struct fsck_options *options)\n+static int mark_object(struct object *obj, enum object_type type,\n+\t\t       void *data, struct fsck_options *options)\n {\n \tstruct object *parent = data;\n \n-- \n2.32.0.rc1.460.g26a014da44c\n\n"},{"id":"426096","messageId":"de9dea3a-6138-5e2f-7dc8-8a2ddf28bb84@kdbg.org","threadId":"55812","inReplyTo":"patch-1.1-f109a61b11-20210601T000433Z-avarab@gmail.com","subject":"Re: [PATCH] builtin/fsck.c: don't conflate \"int\" and \"enum\" in callback","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-06-01T06:25:31Z","receivedAt":"2021-06-01T06:25:37Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 01.06.21 um 02:05 schrieb Ævar Arnfjörð Bjarmason:\n> Fix a warning on AIX's xlc compiler that's been emitted since my\n> a1aad71601a (fsck.h: use \"enum object_type\" instead of \"int\",\n> 2021-03-28):\n> \n>     \"builtin/fsck.c\", line 805.32: 1506-068 (W) Operation between\n>     types \"int(*)(struct object*,enum object_type,void*,struct\n>     fsck_options*)\" and \"int(*)(struct object*,int,void*,struct\n>     fsck_options*)\" is not allowed.\n> \n> I.e. it complains about us assigning a function with a prototype \"int\"\n> where we're expecting \"enum object_type\". Enums are just ints in C,\n> but it seems xlc is more picky than some about conflating them at the\n> source level.\n\nIs that true? I thought compilers were allowed to use whatever data type\nis sufficient to represent all enumeration values. For this reason, you\nsometimes see\n\n   enum X { A, B, X_MAX = 0x7fffffff };\n\nthat ensures that an int must be used for representation of enum X. So,\nAFAICS, your patch is an actual fix, not just cosmetic.\n\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n> \n> This is new in v2.32.0, so sending this during the rc phase, just a\n> warning though, so unless you're using fatal warnings on that\n> OS/platform it won't impact anything, and even then it's just a minor\n> annoyance...\n> \n>  builtin/fsck.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/fsck.c b/builtin/fsck.c\n> index 87a99b0108..b42b6fe21f 100644\n> --- a/builtin/fsck.c\n> +++ b/builtin/fsck.c\n> @@ -109,7 +109,8 @@ static int fsck_error_func(struct fsck_options *o,\n>  \n>  static struct object_array pending;\n>  \n> -static int mark_object(struct object *obj, int type, void *data, struct fsck_options *options)\n> +static int mark_object(struct object *obj, enum object_type type,\n> +\t\t       void *data, struct fsck_options *options)\n>  {\n>  \tstruct object *parent = data;\n>  \n> \n\n"},{"id":"426100","messageId":"87k0ne0zx3.fsf@evledraar.gmail.com","threadId":"55812","inReplyTo":"de9dea3a-6138-5e2f-7dc8-8a2ddf28bb84@kdbg.org","subject":"Re: [PATCH] builtin/fsck.c: don't conflate \"int\" and \"enum\" in callback","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-01T08:04:21Z","receivedAt":"2021-06-01T08:09:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 01 2021, Johannes Sixt wrote:\n\n> Am 01.06.21 um 02:05 schrieb Ævar Arnfjörð Bjarmason:\n>> Fix a warning on AIX's xlc compiler that's been emitted since my\n>> a1aad71601a (fsck.h: use \"enum object_type\" instead of \"int\",\n>> 2021-03-28):\n>> \n>>     \"builtin/fsck.c\", line 805.32: 1506-068 (W) Operation between\n>>     types \"int(*)(struct object*,enum object_type,void*,struct\n>>     fsck_options*)\" and \"int(*)(struct object*,int,void*,struct\n>>     fsck_options*)\" is not allowed.\n>> \n>> I.e. it complains about us assigning a function with a prototype \"int\"\n>> where we're expecting \"enum object_type\". Enums are just ints in C,\n>> but it seems xlc is more picky than some about conflating them at the\n>> source level.\n>\n> Is that true? I thought compilers were allowed to use whatever data type\n> is sufficient to represent all enumeration values. For this reason, you\n> sometimes see\n>\n>    enum X { A, B, X_MAX = 0x7fffffff };\n>\n> that ensures that an int must be used for representation of enum X. So,\n> AFAICS, your patch is an actual fix, not just cosmetic.\n\n\tThe identifiers in an enumerator list are declared as constants\n\tthat have type int and may appear wherever such are\n\tpermitted. [...] Each enumerated type shall be compatible with\n\tchar,asigned integer type, or an unsigned integer type. The\n\tchoice of type is implementation-defined,110) but shall be\n\tcapable of representing the values of all the members of the\n\tenumeration [...] Thus, the identifiers of enumeration constants\n\tdeclared in the same scope shall all be distinct from each other\n\tand from other identifiers declared in ordinary declarators\n\n\t--C99, 6.7.2.2 @\n          http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf\n\nMy reading of that is that mixing the two (which we indeed, do all over\nthe place) is guaranteed to work, we've got plenty of places where\ne.g. enum object_type is passed to something else as an \"int\".\n\nThis xlc warning in particular probably has nothing per-se to do with\nenum v.s. int, but just that it's complaining that a function pointer\ndoesn't have exactly the expected type.\n>> \n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> ---\n>> \n>> This is new in v2.32.0, so sending this during the rc phase, just a\n>> warning though, so unless you're using fatal warnings on that\n>> OS/platform it won't impact anything, and even then it's just a minor\n>> annoyance...\n>> \n>>  builtin/fsck.c | 3 ++-\n>>  1 file changed, 2 insertions(+), 1 deletion(-)\n>> \n>> diff --git a/builtin/fsck.c b/builtin/fsck.c\n>> index 87a99b0108..b42b6fe21f 100644\n>> --- a/builtin/fsck.c\n>> +++ b/builtin/fsck.c\n>> @@ -109,7 +109,8 @@ static int fsck_error_func(struct fsck_options *o,\n>>  \n>>  static struct object_array pending;\n>>  \n>> -static int mark_object(struct object *obj, int type, void *data, struct fsck_options *options)\n>> +static int mark_object(struct object *obj, enum object_type type,\n>> +\t\t       void *data, struct fsck_options *options)\n>>  {\n>>  \tstruct object *parent = data;\n>>  \n>> \n\n"},{"id":"426152","messageId":"8df5f832-adaa-7cb0-38e3-1a4b2bcff252@web.de","threadId":"55812","inReplyTo":"87k0ne0zx3.fsf@evledraar.gmail.com","subject":"Re: [PATCH] builtin/fsck.c: don't conflate \"int\" and \"enum\" in callback","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-01T17:56:33Z","receivedAt":"2021-06-01T17:56:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.06.21 um 10:04 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Tue, Jun 01 2021, Johannes Sixt wrote:\n>\n>> Am 01.06.21 um 02:05 schrieb Ævar Arnfjörð Bjarmason:\n>>> Fix a warning on AIX's xlc compiler that's been emitted since my\n>>> a1aad71601a (fsck.h: use \"enum object_type\" instead of \"int\",\n>>> 2021-03-28):\n>>>\n>>>     \"builtin/fsck.c\", line 805.32: 1506-068 (W) Operation between\n>>>     types \"int(*)(struct object*,enum object_type,void*,struct\n>>>     fsck_options*)\" and \"int(*)(struct object*,int,void*,struct\n>>>     fsck_options*)\" is not allowed.\n>>>\n>>> I.e. it complains about us assigning a function with a prototype \"int\"\n>>> where we're expecting \"enum object_type\". Enums are just ints in C,\n>>> but it seems xlc is more picky than some about conflating them at the\n>>> source level.\n>>\n>> Is that true? I thought compilers were allowed to use whatever data type\n>> is sufficient to represent all enumeration values. For this reason, you\n>> sometimes see\n>>\n>>    enum X { A, B, X_MAX = 0x7fffffff };\n>>\n>> that ensures that an int must be used for representation of enum X. So,\n>> AFAICS, your patch is an actual fix, not just cosmetic.\n>\n> \tThe identifiers in an enumerator list are declared as constants\n> \tthat have type int and may appear wherever such are\n> \tpermitted. [...] Each enumerated type shall be compatible with\n> \tchar,asigned integer type, or an unsigned integer type. The\n> \tchoice of type is implementation-defined,110) but shall be\n> \tcapable of representing the values of all the members of the\n> \tenumeration [...] Thus, the identifiers of enumeration constants\n> \tdeclared in the same scope shall all be distinct from each other\n> \tand from other identifiers declared in ordinary declarators\n>\n> \t--C99, 6.7.2.2 @\n>           http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf\n>\n> My reading of that is that mixing the two (which we indeed, do all over\n> the place) is guaranteed to work, we've got plenty of places where\n> e.g. enum object_type is passed to something else as an \"int\".\n>\n> This xlc warning in particular probably has nothing per-se to do with\n> enum v.s. int, but just that it's complaining that a function pointer\n> doesn't have exactly the expected type.\n\nThe object_type item OBJ_BAD has the value -1.  If it had a low positive\nvalue instead, GCC and Clang would warn.  Demo:\nhttps://godbolt.org/z/vKPdjrYsa\n\nAs you cited above, \"The choice of type is implementation-defined\".  You\ncan use enum values as if they were integers, but that doesn't dictate\ntheir storage size.\n\nI find the lack of a warning depending on the value range disturbing.\nPerhaps it's omitted because GCC and Clang guarantee compatibility of\nsuch an enum and int for all prior versions (i.e. the implementation-\ndefined specifics never changed).  A stricter check like xlc does is\nmore useful for portability, though.\n\n>>>\n>>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>>> ---\n>>>\n>>> This is new in v2.32.0, so sending this during the rc phase, just a\n>>> warning though, so unless you're using fatal warnings on that\n>>> OS/platform it won't impact anything, and even then it's just a minor\n>>> annoyance...\n>>>\n>>>  builtin/fsck.c | 3 ++-\n>>>  1 file changed, 2 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/builtin/fsck.c b/builtin/fsck.c\n>>> index 87a99b0108..b42b6fe21f 100644\n>>> --- a/builtin/fsck.c\n>>> +++ b/builtin/fsck.c\n>>> @@ -109,7 +109,8 @@ static int fsck_error_func(struct fsck_options *o,\n>>>\n>>>  static struct object_array pending;\n>>>\n>>> -static int mark_object(struct object *obj, int type, void *data, struct fsck_options *options)\n>>> +static int mark_object(struct object *obj, enum object_type type,\n>>> +\t\t       void *data, struct fsck_options *options)\n>>>  {\n>>>  \tstruct object *parent = data;\n>>>\n>>>\n>\n\n"},{"id":"426162","messageId":"xmqq8s3ts3ds.fsf@gitster.g","threadId":"55812","inReplyTo":"patch-1.1-f109a61b11-20210601T000433Z-avarab@gmail.com","subject":"Re: [PATCH] builtin/fsck.c: don't conflate \"int\" and \"enum\" in callback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-01T21:04:31Z","receivedAt":"2021-06-01T21:04:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Fix a warning on AIX's xlc compiler that's been emitted since my\n> a1aad71601a (fsck.h: use \"enum object_type\" instead of \"int\",\n> 2021-03-28):\n>\n>     \"builtin/fsck.c\", line 805.32: 1506-068 (W) Operation between\n>     types \"int(*)(struct object*,enum object_type,void*,struct\n>     fsck_options*)\" and \"int(*)(struct object*,int,void*,struct\n>     fsck_options*)\" is not allowed.\n>\n> I.e. it complains about us assigning a function with a prototype \"int\"\n> where we're expecting \"enum object_type\". Enums are just ints in C,\n> but it seems xlc is more picky than some about conflating them at the\n> source level.\n\nAs others already have pointed out, xlc is correct and these\nwarnings are perhaps useful.  I'd just remove the last sentence\nwhile queuing, as \"Enums are just ints in C\" is only half a truth.\n\nThe constants like OBJ_COMMIT can be used as \"int\", but the type of\nan enum itself may not be the same as \"int\"---it can be narrower.\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>\n> This is new in v2.32.0, so sending this during the rc phase, just a\n> warning though, so unless you're using fatal warnings on that\n> OS/platform it won't impact anything, and even then it's just a minor\n> annoyance...\n\nThanks.\n\n>\n>  builtin/fsck.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/fsck.c b/builtin/fsck.c\n> index 87a99b0108..b42b6fe21f 100644\n> --- a/builtin/fsck.c\n> +++ b/builtin/fsck.c\n> @@ -109,7 +109,8 @@ static int fsck_error_func(struct fsck_options *o,\n>  \n>  static struct object_array pending;\n>  \n> -static int mark_object(struct object *obj, int type, void *data, struct fsck_options *options)\n> +static int mark_object(struct object *obj, enum object_type type,\n> +\t\t       void *data, struct fsck_options *options)\n>  {\n>  \tstruct object *parent = data;\n"}]}