Re: bitpos expansion patches summary

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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