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

[Indu Bhagat]

On 12/8/22 09:43, Indu Bhagat wrote:
> 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.

[ And I Misspoke :) Sorry, I mixed up the fre_type with the stack 
offsets type. The latter has 2-bits reserved to encode 3 possible values 
(1 byte/2 byte/4 bytes); See "size of offsets" in fre_info. ]

Correction - There are 4-bits reserved for encoding the FRE types. At 
the moment there are 3 types of SFrame FREs being used (leaving space 
for the format to add other FRE types if needed). So the constants 
SFRAME_FRE_TYPE_ADDR1, SFRAME_FRE_TYPE_ADDR2, and SFRAME_FRE_TYPE_ADDR3 
are defined to use up the available space serially. Hence the weirdness 
in the *_LIMIT formulae.