[Cbe-oss-dev] [PATCH 02/11]MARS: Workload queue block replace bit fields

[Yuji Mano]

Kazunori Asayama wrote:
> Yuji Mano wrote:
>> Kazunori Asayama wrote:
>>> Yuji Mano wrote:
>>>> This replaces the bit fields usage for the workload queue block bits with
>>>> explicit bitwise shift/mask operations for better portability.
>>>>
>>>> Signed-off-by: Yuji Mano <yuji.mano at am.sony.com>
>>> (snip)
>>>
>>>> +#define MARS_BITS_CAST_TYPE			uint8_t
>>>> +#define MARS_BITS_CAST_STATE			uint8_t
>>>> +#define MARS_BITS_CAST_PRIORITY			uint8_t
>>>> +#define MARS_BITS_CAST_SIGNAL			uint8_t
>>>> +#define MARS_BITS_CAST_WAIT_ID			uint16_t
>>>> +#define MARS_BITS_CAST_COUNTER			uint16_t
>>>> +
>>>> +#define MARS_BITS_GET(bits, name) (MARS_BITS_CAST_##name) \
>>>> +	((*(bits) & MARS_BITS_MASK_##name) >> MARS_BITS_SHIFT_##name)
>>> Why are these explicit casts needed?
>> 
>> I guess they may not be absolutely necessary...
>> But I think it also makes it more clear what integer type you should store
>> the value in when needed.
> 
> But the cast operation is completely hidden from the code by the macro
> MARS_BITS_GET(). Additionally, I don't think types are enough to
> represent the range of the fields. That is, e.g., actually TYPE is 4bits
> length, but the cast is 'uint8_t'.
> 
> And in mars_kernel_scheduler.c, for example 'priority' is always casted
> from 'uint8_t' to 'int', so I'm not sure the PRIORITY should be 'uint8_t'.

Ok. I will remove the explicit casts.

Also I will remove the casting of 'priority' to int in the scheduler in the v2 of this patch.

>> 
>> Also for example if you try to do aomething like the following without the cast:
>> 
>> printf("priority = %u\n", MARS_BITS_GET(&bits, PRIORITY);
>> 
>> you would get the following warning:
>> 
>> warning: format '%u' expects type 'unsigned int', but argument 2 has type 'long long unsigned int
> 
> Or you can use '%llu', instead?
> 

Yes we can. Anyway this would only be for internal debugging purposes anyway so it is a minor issue.

Regards,
Yuji