Re: [PATCH v2 5/7] Adds support for thumb32 instructions recording

[Pedro Alves <palves@redhat.com>]

On 01/03/2014 07:15 PM, Omair Javaid wrote:
> +  switch (op1)
> +    {
> +      /* Store byte instructions.  */
> +      case 4:
> +      case 0:
> +        record_buf_mem[0] = 1;
> +      break;

"break;" should be aligned with the above statement,
not with "case".  Several instances of this.

> +      /* Swap first half of 32bit thumb instruction with second half.  */
> +      arm_record->arm_insn = (arm_record->arm_insn >> 16) |
> +                             (arm_record->arm_insn << 16);

'|' goes on the next line. But then you'd need to wrap the whole
thing with an extra set of parens.  Write instead:

      arm_record->arm_insn
         = (arm_record->arm_insn >> 16) | (arm_record->arm_insn << 16);

> +
> +      insn_id = thumb2_record_decode_insn_handler (arm_record);
> +
> +      if (insn_id != ARM_RECORD_SUCCESS)
> +        {
> +          arm_record_unsupported_insn(arm_record);

Missing space before parens.

> +          ret = -1;
> +        }
>      }
>    else

Otherwise looks fine to me.

Thanks,
-- 
Pedro Alves