Re: [PATCH 3/6] sframe: gas: libsframe: define constants and remove magic numbers

[Indu Bhagat <indu.bhagat@oracle.com>]

On 12/8/22 03:10, Nick Clifton wrote:
> Hi Indu,
> 
>> +#define SFRAME_FRE_TYPE_ADDR1_LIMIT  ((SFRAME_FRE_TYPE_ADDR1+1)*8)
> 
> For readabilities sake, I would recommend adding whitespace around 
> arithmetic
> operations.  For example in the define above a quick glance would 
> suggest that
> the definition is for a symbol called ...ADDR11 rather than ...ADDR1 + 1.
> So:
> 
> #define SFRAME_FRE_TYPE_ADDR1_LIMIT  ((SFRAME_FRE_TYPE_ADDR1 + 1) * 8)
> 
> Is better IMHO.
> 

Yes, I agree. I will fix it.

>> +#define SFRAME_FRE_TYPE_ADDR2_LIMIT  ((SFRAME_FRE_TYPE_ADDR2*2)*8)
>> +#define SFRAME_FRE_TYPE_ADDR4_LIMIT  ((SFRAME_FRE_TYPE_ADDR4*2)*8)
> 
> The same goes for these two definitions as well.
> 
> Patch approved with these changes.
> 
> Cheers
>    Nick
> 
> PS.  Just checking, since I am not actually familiar with the sframe
> format:  Is it correct that SFRAME_FRE_TYPE_ADDR1_LIMIT is defined
> as "(...ADDR1 + 1) * 8" rather than "(...ADDR1 * 2) * 8)" ?  It is
> just that the other two limits are defined using the second formula
> and it seems slightly odd that it is not used for the first.
> 

Yes, it does appear unpleasing to the eye. But it is the way it is 
because the constants are defined as following:

#define SFRAME_FRE_TYPE_ADDR1   0
#define SFRAME_FRE_TYPE_ADDR2   1
#define SFRAME_FRE_TYPE_ADDR4   2

All this scrambling because keeping 3-bits was deemed sufficient to 
encode 3 different values (size of 1 byte, 2 byte and 4 bytes 
respectively), and the rest of the bits were provisioned for other 
information.

Thanks for reviewing,
Indu