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