Re: [PATCH] arm reversible : <phase_2_complete>

[Petr HluzÃn <petr dot hluzin at gmail dot com>]

I did not review the 2011-11-19 nor 2011-11-09 patch.

Tom:
> Yeah, it is a bit of a pain.  I wish we had a tool that could reformat
> the code according to our standards.

I guess 'indent' could do that. Its default settings formats using GNU
guidelines.
Some editors can be configured to automatically format new lines and
have a button to reformat existing lines. I suppose no editor can
autodetect the style from file yet.


Review of the 2011-11-03 patch:

In arm_process_record():
Expression `insn_id = bits (arm_record.arm_insn, 11, 15);' uses value
of `arm_record.arm_insn'
which is always zero. When you moved the test from decode_insn() I
guess forgot to copy these lines:
+      arm_record->arm_insn = (uint32_t) extract_unsigned_integer
(&u_buf.buf[0],
+           THUMB_INSN_SIZE_BYTES, gdbarch_byte_order (arm_record->gdbarch));


Remaining from previous reviews:

Macro INSN_RECORDED:
I suggested INSN_IS_RECORDED to make it clear it is boolean, Tom
Tromey did not comment on that, Oza left the name as is. Well, it is
not a big deal.
However arguments of macros should be surrounded by parentheses - like this:
(ARM_RECORD)->reg_rec_count
This is necessary if INSN_RECORDED() is passed more complex expression
when the order of evaluation matters. This is a common C issue and
customary solution, it is not GDB specific.

> Petr >>In arm_record_strx()
> Petr >>Why don't you use `record_buf_mem[0]' and `record_buf_mem[1]' syntax?
>
> because calling function takes care of REG_ALLOC routine calls.
> I did not want ALLOC calls to be scattered into any other function
> other than main decoding functions.

The lines can be converted from `*(record_buf_mem + 1)' to
`record_buf_mem[1]' with no changes to REG_ALLOC() calls. The
allocation calls would remain unscattered in main decoding functions.
The code is correct, but I do not understand why you use the unusual
syntax.

Have you considered adding the assertions I suggested in [2]? They are
not necessary but they improve understanding of code.

[2] http://sourceware.org/ml/gdb-patches/2011-10/msg00617.html

-- 
Petr Hluzin