[PATCH] builtin/fsck.c: don't conflate "int" and "enum" in callback

[PATCH] builtin/fsck.c: don't conflate "int" and "enum" in callback

[Ævar Arnfjörð Bjarmason]

Fix a warning on AIX's xlc compiler that's been emitted since my
a1aad71601a (fsck.h: use "enum object_type" instead of "int",
2021-03-28):

    "builtin/fsck.c", line 805.32: 1506-068 (W) Operation between
    types "int(*)(struct object*,enum object_type,void*,struct
    fsck_options*)" and "int(*)(struct object*,int,void*,struct
    fsck_options*)" is not allowed.

I.e. it complains about us assigning a function with a prototype "int"
where we're expecting "enum object_type". Enums are just ints in C,
but it seems xlc is more picky than some about conflating them at the
source level.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

This is new in v2.32.0, so sending this during the rc phase, just a
warning though, so unless you're using fatal warnings on that
OS/platform it won't impact anything, and even then it's just a minor
annoyance...

 builtin/fsck.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 87a99b0108..b42b6fe21f 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -109,7 +109,8 @@ static int fsck_error_func(struct fsck_options *o,
 
 static struct object_array pending;
 
-static int mark_object(struct object *obj, int type, void *data, struct fsck_options *options)
+static int mark_object(struct object *obj, enum object_type type,
+		       void *data, struct fsck_options *options)
 {
 	struct object *parent = data;
 
-- 
2.32.0.rc1.460.g26a014da44c

Re: [PATCH] builtin/fsck.c: don't conflate "int" and "enum" in callback

[Johannes Sixt]

Am 01.06.21 um 02:05 schrieb Ævar Arnfjörð Bjarmason:
> Fix a warning on AIX's xlc compiler that's been emitted since my
> a1aad71601a (fsck.h: use "enum object_type" instead of "int",
> 2021-03-28):
> 
>     "builtin/fsck.c", line 805.32: 1506-068 (W) Operation between
>     types "int(*)(struct object*,enum object_type,void*,struct
>     fsck_options*)" and "int(*)(struct object*,int,void*,struct
>     fsck_options*)" is not allowed.
> 
> I.e. it complains about us assigning a function with a prototype "int"
> where we're expecting "enum object_type". Enums are just ints in C,
> but it seems xlc is more picky than some about conflating them at the
> source level.

Is that true? I thought compilers were allowed to use whatever data type
is sufficient to represent all enumeration values. For this reason, you
sometimes see

   enum X { A, B, X_MAX = 0x7fffffff };

that ensures that an int must be used for representation of enum X. So,
AFAICS, your patch is an actual fix, not just cosmetic.

> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 
> This is new in v2.32.0, so sending this during the rc phase, just a
> warning though, so unless you're using fatal warnings on that
> OS/platform it won't impact anything, and even then it's just a minor
> annoyance...
> 
>  builtin/fsck.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 87a99b0108..b42b6fe21f 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -109,7 +109,8 @@ static int fsck_error_func(struct fsck_options *o,
>  
>  static struct object_array pending;
>  
> -static int mark_object(struct object *obj, int type, void *data, struct fsck_options *options)
> +static int mark_object(struct object *obj, enum object_type type,
> +		       void *data, struct fsck_options *options)
>  {
>  	struct object *parent = data;
>  
> 

Re: [PATCH] builtin/fsck.c: don't conflate "int" and "enum" in callback

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 01 2021, Johannes Sixt wrote:

> Am 01.06.21 um 02:05 schrieb Ævar Arnfjörð Bjarmason:
>> Fix a warning on AIX's xlc compiler that's been emitted since my
>> a1aad71601a (fsck.h: use "enum object_type" instead of "int",
>> 2021-03-28):
>> 
>>     "builtin/fsck.c", line 805.32: 1506-068 (W) Operation between
>>     types "int(*)(struct object*,enum object_type,void*,struct
>>     fsck_options*)" and "int(*)(struct object*,int,void*,struct
>>     fsck_options*)" is not allowed.
>> 
>> I.e. it complains about us assigning a function with a prototype "int"
>> where we're expecting "enum object_type". Enums are just ints in C,
>> but it seems xlc is more picky than some about conflating them at the
>> source level.
>
> Is that true? I thought compilers were allowed to use whatever data type
> is sufficient to represent all enumeration values. For this reason, you
> sometimes see
>
>    enum X { A, B, X_MAX = 0x7fffffff };
>
> that ensures that an int must be used for representation of enum X. So,
> AFAICS, your patch is an actual fix, not just cosmetic.

	The identifiers in an enumerator list are declared as constants
	that have type int and may appear wherever such are
	permitted. [...] Each enumerated type shall be compatible with
	char,asigned integer type, or an unsigned integer type. The
	choice of type is implementation-defined,110) but shall be
	capable of representing the values of all the members of the
	enumeration [...] Thus, the identifiers of enumeration constants
	declared in the same scope shall all be distinct from each other
	and from other identifiers declared in ordinary declarators

	--C99, 6.7.2.2 @
          http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

My reading of that is that mixing the two (which we indeed, do all over
the place) is guaranteed to work, we've got plenty of places where
e.g. enum object_type is passed to something else as an "int".

This xlc warning in particular probably has nothing per-se to do with
enum v.s. int, but just that it's complaining that a function pointer
doesn't have exactly the expected type.
>> 
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> 
>> This is new in v2.32.0, so sending this during the rc phase, just a
>> warning though, so unless you're using fatal warnings on that
>> OS/platform it won't impact anything, and even then it's just a minor
>> annoyance...
>> 
>>  builtin/fsck.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/builtin/fsck.c b/builtin/fsck.c
>> index 87a99b0108..b42b6fe21f 100644
>> --- a/builtin/fsck.c
>> +++ b/builtin/fsck.c
>> @@ -109,7 +109,8 @@ static int fsck_error_func(struct fsck_options *o,
>>  
>>  static struct object_array pending;
>>  
>> -static int mark_object(struct object *obj, int type, void *data, struct fsck_options *options)
>> +static int mark_object(struct object *obj, enum object_type type,
>> +		       void *data, struct fsck_options *options)
>>  {
>>  	struct object *parent = data;
>>  
>> 

Re: [PATCH] builtin/fsck.c: don't conflate "int" and "enum" in callback

[René Scharfe]

Am 01.06.21 um 10:04 schrieb Ævar Arnfjörð Bjarmason:
>
> On Tue, Jun 01 2021, Johannes Sixt wrote:
>
>> Am 01.06.21 um 02:05 schrieb Ævar Arnfjörð Bjarmason:
>>> Fix a warning on AIX's xlc compiler that's been emitted since my
>>> a1aad71601a (fsck.h: use "enum object_type" instead of "int",
>>> 2021-03-28):
>>>
>>>     "builtin/fsck.c", line 805.32: 1506-068 (W) Operation between
>>>     types "int(*)(struct object*,enum object_type,void*,struct
>>>     fsck_options*)" and "int(*)(struct object*,int,void*,struct
>>>     fsck_options*)" is not allowed.
>>>
>>> I.e. it complains about us assigning a function with a prototype "int"
>>> where we're expecting "enum object_type". Enums are just ints in C,
>>> but it seems xlc is more picky than some about conflating them at the
>>> source level.
>>
>> Is that true? I thought compilers were allowed to use whatever data type
>> is sufficient to represent all enumeration values. For this reason, you
>> sometimes see
>>
>>    enum X { A, B, X_MAX = 0x7fffffff };
>>
>> that ensures that an int must be used for representation of enum X. So,
>> AFAICS, your patch is an actual fix, not just cosmetic.
>
> 	The identifiers in an enumerator list are declared as constants
> 	that have type int and may appear wherever such are
> 	permitted. [...] Each enumerated type shall be compatible with
> 	char,asigned integer type, or an unsigned integer type. The
> 	choice of type is implementation-defined,110) but shall be
> 	capable of representing the values of all the members of the
> 	enumeration [...] Thus, the identifiers of enumeration constants
> 	declared in the same scope shall all be distinct from each other
> 	and from other identifiers declared in ordinary declarators
>
> 	--C99, 6.7.2.2 @
>           http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
>
> My reading of that is that mixing the two (which we indeed, do all over
> the place) is guaranteed to work, we've got plenty of places where
> e.g. enum object_type is passed to something else as an "int".
>
> This xlc warning in particular probably has nothing per-se to do with
> enum v.s. int, but just that it's complaining that a function pointer
> doesn't have exactly the expected type.

The object_type item OBJ_BAD has the value -1.  If it had a low positive
value instead, GCC and Clang would warn.  Demo:
https://godbolt.org/z/vKPdjrYsa

As you cited above, "The choice of type is implementation-defined".  You
can use enum values as if they were integers, but that doesn't dictate
their storage size.

I find the lack of a warning depending on the value range disturbing.
Perhaps it's omitted because GCC and Clang guarantee compatibility of
such an enum and int for all prior versions (i.e. the implementation-
defined specifics never changed).  A stricter check like xlc does is
more useful for portability, though.

>>>
>>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>> ---
>>>
>>> This is new in v2.32.0, so sending this during the rc phase, just a
>>> warning though, so unless you're using fatal warnings on that
>>> OS/platform it won't impact anything, and even then it's just a minor
>>> annoyance...
>>>
>>>  builtin/fsck.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/builtin/fsck.c b/builtin/fsck.c
>>> index 87a99b0108..b42b6fe21f 100644
>>> --- a/builtin/fsck.c
>>> +++ b/builtin/fsck.c
>>> @@ -109,7 +109,8 @@ static int fsck_error_func(struct fsck_options *o,
>>>
>>>  static struct object_array pending;
>>>
>>> -static int mark_object(struct object *obj, int type, void *data, struct fsck_options *options)
>>> +static int mark_object(struct object *obj, enum object_type type,
>>> +		       void *data, struct fsck_options *options)
>>>  {
>>>  	struct object *parent = data;
>>>
>>>
>

Re: [PATCH] builtin/fsck.c: don't conflate "int" and "enum" in callback

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Fix a warning on AIX's xlc compiler that's been emitted since my
> a1aad71601a (fsck.h: use "enum object_type" instead of "int",
> 2021-03-28):
>
>     "builtin/fsck.c", line 805.32: 1506-068 (W) Operation between
>     types "int(*)(struct object*,enum object_type,void*,struct
>     fsck_options*)" and "int(*)(struct object*,int,void*,struct
>     fsck_options*)" is not allowed.
>
> I.e. it complains about us assigning a function with a prototype "int"
> where we're expecting "enum object_type". Enums are just ints in C,
> but it seems xlc is more picky than some about conflating them at the
> source level.

As others already have pointed out, xlc is correct and these
warnings are perhaps useful.  I'd just remove the last sentence
while queuing, as "Enums are just ints in C" is only half a truth.

The constants like OBJ_COMMIT can be used as "int", but the type of
an enum itself may not be the same as "int"---it can be narrower.

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> This is new in v2.32.0, so sending this during the rc phase, just a
> warning though, so unless you're using fatal warnings on that
> OS/platform it won't impact anything, and even then it's just a minor
> annoyance...

Thanks.

>
>  builtin/fsck.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 87a99b0108..b42b6fe21f 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -109,7 +109,8 @@ static int fsck_error_func(struct fsck_options *o,
>  
>  static struct object_array pending;
>  
> -static int mark_object(struct object *obj, int type, void *data, struct fsck_options *options)
> +static int mark_object(struct object *obj, enum object_type type,
> +		       void *data, struct fsck_options *options)
>  {
>  	struct object *parent = data;