Re: [Qemu-devel] [PULL 09/11] tcg-aarch64: Use 32-bit loads for qemu_ld_i32

[Claudio Fontana]

On 24.09.2014 17:19, Richard Henderson wrote:
> On 09/24/2014 01:20 AM, Claudio Fontana wrote:
>>> @@ -1118,7 +1119,8 @@ static void tcg_out_qemu_ld_direct(TCGContext *s, 
>>> TCGMemOp memop,
>>>          tcg_out_ldst_r(s, I3312_LDRB, data_r, addr_r, off_r);
>>>          break;
>>>      case MO_SB:
>>> -        tcg_out_ldst_r(s, I3312_LDRSBX, data_r, addr_r, off_r);
>>> +        tcg_out_ldst_r(s, type ? I3312_LDRSBX : I3312_LDRSBW,
>>> +                       data_r, addr_r, off_r);
>>
>>
>> since we are using the enum type TCGType, why do we check type as "type ?"
>>
>> I would have expected the conditional to be something like
>>
>> type == TCG_TYPE_I32 ? I3312_LDRSBW : I3312_LDRSBX
>>
>> It's pretty obvious what is happening but it might spare someone a lookup 
>> into the header file
>> to test that type 0 is indeed TCG_TYPE_I32.
> 
> We assert the boolean-ish nature of TCGType at the start of the file, and use
> it for the "ext" variable throughout.  Would it help if the variable weren't
> named "type"?
> 
> 
> r~

I could not find a comment describing that in tcg.h, did I miss it in the patch?

I find it difficult to come up with a better name for TCGType, I wonder what 
that could be..

but what about not caring about the boolish nature of TCGType and instead
check it explicitly? I understand that there are multiple enumeration constants
with the same value, but if the names of the first two enumeration constants are
general enough, that should be understandable, no?

A comment before TCGType could explain this, and then we could have values like

TCG_TYPE_32 = 0,
TCG_TYPE_64,

/* .. all the other aliases */

Which is basically what we have now isn't it? TCG_TYPE_I32 and TCG_TYPE_I64 are
general enough I think, so why aren't we checking those constants explicitly?

Ciao,

Claudio