Re: [PATCH v2 1/2] common: move Linux-inherited fixed width type decls to common header

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.08.2023 12:49, Julien Grall wrote:
> On 04/08/2023 07:08, Jan Beulich wrote:
>> Have these in one place, for all architectures to use. Also use the C99
>> types as the "original" ones, and derive the Linux compatible ones
>> (which we're trying to phase out). For __s<N>, seeing that no uses exist
>> anymore, move them to a new Linux compatibility header (as an act of
>> precaution - as said, we don't have any uses of these types right now).
>>
>> In some Flask sources inclusion of asm/byteorder.h needs moving later.
>>
>> No functional change intended.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> With one remark:
> 
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

Thanks.

>> --- a/xen/arch/arm/include/asm/types.h
>> +++ b/xen/arch/arm/include/asm/types.h
>> @@ -1,18 +1,7 @@
>>   #ifndef __ARM_TYPES_H__
>>   #define __ARM_TYPES_H__
>>   
>> -typedef signed char s8;
>> -typedef unsigned char u8;
>> -
>> -typedef signed short s16;
>> -typedef unsigned short u16;
>> -
>> -typedef signed int s32;
>> -typedef unsigned int u32;
>> -
>>   #if defined(CONFIG_ARM_32)
>> -typedef signed long long s64;
>> -typedef unsigned long long u64;
>>   typedef u32 vaddr_t;
>>   #define PRIvaddr PRIx32
>>   #if defined(CONFIG_PHYS_ADDR_T_32)
>> @@ -34,9 +23,9 @@ typedef u64 paddr_t;
>>   #endif
> 
> I find odd that you don't add a newline before/after #endif here but...
> 
>>   typedef u32 register_t;
>>   #define PRIregister "08x"
>> -#elif defined (CONFIG_ARM_64)
>> -typedef signed long s64;
>> -typedef unsigned long u64;
>> +
>> +#elif defined(CONFIG_ARM_64)
>> +
>>   typedef u64 vaddr_t;
>>   #define PRIvaddr PRIx64
>>   typedef u64 paddr_t;
>> @@ -44,6 +33,7 @@ typedef u64 paddr_t;
>>   #define PRIpaddr "016lx"
>>   typedef u64 register_t;
>>   #define PRIregister "016lx"
>> +
> 
> .. you add one here. But this change looks a bit unrelated and IMHO of 
> out context.
> 
>>   #endif
>>   
>>   #endif /* __ARM_TYPES_H__ */

Hmm, I think I was mistaking the blank line after

#if defined(CONFIG_PHYS_ADDR_T_32)

as the matching one. Which way would you prefer me to resolve the
inconsistency: Add the missing blank line, or drop again the three ones
I'm presently adding?

Jan