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

[Yao Qi <qiyaoltc@gmail.com>]

Simon Marchi <simon.marchi@ericsson.com> writes:

> 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)

Yes, I clearly understand your goal, but I don't think the change is
necessary.  However, I can't see anything harmful or negative in this
patch, and looks the patch is useful in terms of helping you reference
the doc easily, I am OK.

>
> 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.

but if you write code like "if (bits (insn, 28, 31) != INST_NV)", people
do understand 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?

NV means Never.

>> 
>> '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.

OK, that is fine, since this is the personal flavour of writing code.

-- 
Yao (齐尧)