Re: [PATCH] arm reversible : <phase_2_complete>

[oza Pawandeep <oza dot pawandeep at gmail dot com>]

Hi Tom,

Thanks for the comments.

1) I have been trying to fix all formatting, it looks better now and
fixed all comments you had.
I am sorry that on formatting both of our efforts are going;
excuse me as every time mailer spoils the things; probably copy from
xemacs editor to chrome/firefox in plain text and the other mailer
gets it. though I am not sure.

2) I could not write changelog entry because I have been getting
commments, and moreover I was not sure whether I should get changelog
entry from latest gdb 7.3 or latest current working version of gdb ? I
think later is true in my understanding.
I will include changelog this time; sorry for having you point it out
in all reviews.

3) will post the latest patch, with possible correction of formatting
issues; and will include changelog from current branch.

Regards,
Oza.



On Sat, Dec 3, 2011 at 12:06 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "oza" == oza Pawandeep <oza.pawandeep@gmail.com> writes:
>
> oza> please find the updated patch; I have tried my level best to take care
> oza> the most to get formatting right.
> oza> And also, all extra parentheses removed.
>
> Thanks.
>
> oza> please have a look. ?(hope mailer does not do anything)
>
> Your mailer is still screwed up.
>
> You didn't write a ChangeLog entry or a NEWS entry.
> Please do this. ?I have asked at least once now. ?These are just
> baseline things for getting patches into gdb; if you don't respond to
> these requests it makes me tend to prioritize other patches over yours.
> FYI.
>
> oza> + ? ? ?/* if this insn has fallen into extension space
> oza> + ? ? ? ? then we need not decode it anymore. ?*/
> oza> + ? ? ?if (ret != -1 && !INSN_RECORDED(arm_record))
> oza> + ? ? ? ?{
> oza> + ? ? ? ? ?arm_handle_insn[insn_id] (arm_record);
>
> Still not checking the return value.
>
> oza> +/* cleans up local record registers and memory allocations. ?*/
>
> Should start with a capital letter.
> Blank line between comment and function.
>
> oza> +void deallocate_reg_mem(insn_decode_record *record)
>
> Newline after void.
> Space before open paren.
>
> oza> +{
> oza> + ?if (record->arm_regs)
> oza> + ? ?xfree (record->arm_regs);
> oza> + ?if (record->arm_mems)
> oza> + ? ?xfree (record->arm_mems);
> oza> +}
>
> You can call xfree on a NULL pointer. ?Just remove those ifs.
>
> oza> + ? ? ? ? ?for (no_of_rec = 0; no_of_rec < arm_record.mem_rec_count;
> oza> no_of_rec++)
> oza> + ? ? ? ? ?{
>
> Brace indentation looks wrong.
>
> There are still other minor formatting issues. ?I saw 21 places where
> there is a comma at the start of a line, please fix all of these.
> I think there were some other problems, too, but at this point I have
> stopped caring.
>
> Tom