Re: bitpos expansion patches summary

Jan Kratochvil
2012-09-11T19:37:53Z

On Tue, 11 Sep 2012 21:26:06 +0200, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
>
> Jan> Additionally I am pretty sure the codebase will get broken soon
> Jan> again as it is common GDB practice to use 'int' for every length
> Jan> and I do not review very every check-in. So it would be nice to
> Jan> possibly be able to do such incremental re-checks in the future;
> Jan> although I am not sure it will be done.
>
> Perhaps you could post something here describing your intended rules.

Yes, I will try to make some summary for further reviews.

> It could be part of patch review. That obviously won't catch
> everything, but we can make an effort at least.

Yes, it should be.

> Jan> Also IMO (any feedback from other maintainers?) we need full
> Jan> annotation of the patch file as with such large number of change
> Jan> there is not clear which changes are justified and whether there
> Jan> are no excessive changes.
> Jan> http://people.redhat.com/jkratoch/bitpos3.patch
> Jan> (lines starting with 'x')
>
> Ouch, 168 hits.
>
> I guess I'm not really sure what you mean by a full annotation.

$ wget -q -O - http://people.redhat.com/jkratoch/bitpos3.patch|perl -lne '$pn=/^[+](?![+][+] )/;(/^x/?$done:$missing)++ if $p&&!$pn;$p=$pn;END{print "done=$done,missing=$missing";}'
done=120,missing=574

I have annotated 120 changes but still 574 changes need to be annotated.
(A few of them were commented by a suggestion for their removal etc.)

This case I call 'done', annotated:

/* Append a trace_quick instruction to EXPR, to record N bytes. */
-extern void ax_trace_quick (struct agent_expr *EXPR, int N);
+extern void ax_trace_quick (struct agent_expr *EXPR, LONGEST N);
xFIXED(Expand N to LONGEST): (ax-gdb.c:516): FUNC(ax_trace_quick): (ULONGEST to int) [(type)->length]

This case I call 'missing', not yet annotated:
long reg_r0, reg_r1, reg_r2;
- int total_len = 0;
+ LONGEST total_len = 0;
enum bfin_abi abi = bfin_abi (gdbarch);

Thanks,
Jan