Re: [PATCH 1/3] arm-tdep.c: Refactor arm_process_displaced_insn

[Simon Marchi <simon dot marchi at ericsson dot com>]

On 16-02-11 06:21 AM, Yao Qi wrote:
> Simon Marchi <simon.marchi@ericsson.com> writes:
> 
>> -  if ((insn & 0xf0000000) == 0xf0000000)
>> -    err = arm_decode_unconditional (gdbarch, insn, regs, dsc);
>> -  else switch (((insn & 0x10) >> 4) | ((insn & 0xe000000) >> 24))
>> +  cond = bits (insn, 28, 31);
> 
> Variable 'cond' is only used once, so don't need to define it.  This is
> my personal flavour.

Well, my goal was to use variables with names that refer to these tables:

http://nova.polymtl.ca/~simark/ss/fileJVxJNx.png
(ARM Architecture Reference Manual, section A5.1)

If you only use the bits (insn, 28, 31) notation, I think you lose readability,
because the you have to do one more indirection in the doc, to go see what those
bits mean.

>> +  op1 = bits (insn, 25, 27);
>> +  op = bit (insn, 4);
>> +
>> +  if (cond != 0xf)
> 
> if (bits (insn, 28, 31) != INST_NV)
> 
> this is consistent with other places in arm-tdep.c

I agree, if there is a define for that it should be used.  What does _NV stand
for though?

>>      {
>> -    case 0x0: case 0x1: case 0x2: case 0x3:
>> -      err = arm_decode_dp_misc (gdbarch, insn, regs, dsc);
>> -      break;
>> +      switch (op1)
>> +	{
>> +	case 0x0:
>> +	case 0x1:
>> +	  -/* Data-processing and miscellaneous instructions  */
>> +	  err = arm_decode_dp_misc (gdbarch, insn, regs, dsc);
>> +	  break;
>>  
>> -    case 0x4: case 0x5: case 0x6:
>> -      err = arm_decode_ld_st_word_ubyte (gdbarch, insn, regs, dsc);
>> -      break;
>> +	case 0x2:
>> +	  /* Load/store word and unsigned byte  */
>> +	  err = arm_decode_ld_st_word_ubyte (gdbarch, insn, regs, dsc);
>> +	  break;
>>  
>> -    case 0x7:
>> -      err = arm_decode_media (gdbarch, insn, dsc);
>> -      break;
>> +	case 0x3:
>> +	  if (op == 0)
> 
> 'op' is only used here, let us define it in this block, or use
> 'bit (insn, 4)' instead.

Ok for moving it, but I would suggest keeping the variable op, for
the same reason as cond mentioned above.

Thanks,

Simon