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

[Kazunori Asayama]

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'.

> 
> 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?

-- 
(ASAYAMA Kazunori
  (asayama at sm.sony.co.jp))
t