Re: Patch review process

[Daniel Berlin <dan at cgsoftware dot com>]

Andrew Cagney <ac131313@cygnus.com> writes:

>> I don't have any magic wand to offer, but one way to improve things
>> is to (1) reply to posted patches quickly, even if the reply just
>> says ``noted, will get to it soon''; and (2) review large patches in
>> small chunks and publish any requests to modify the reviewed parts
>> as soon as you can say something useful.  That still leaves the more
>> general design issues that cannot be reviewed without allocating
>> significant time, but at least it gets the minor boring issues such
>> as formatting, ChangeLog entries, etc. out of your way.
> 
> 
> 
> It should be possible to take some of these factors out of the
> equation. For instance, both tracking patches and needing to comment
> on stylistic issues.
> 
> 
> Aegis, for instance, handles the adminstrative side of a patch
> submition.  Before a change even surfaces it has been put through some
> basic checking criteria ex: does it compile; does it meet certain
> criteria of the coding standard; if it fixes a bug does it include a
> test case that pass/fails dependant on the change; ... (you could
> quickly get out of control here :-).  Once this basic criteria have
> been met, it is passed on to the relevant maintainer for approval.
> Once approved, much of the commit phase is also handled automatically.
> The system always knows what patches are where.
> 
> 
> Aegis, in our environment, however won't fly - at a technical level it
> isn't very good at being distributed distributed.
> 
> 
> While trying to build an equivalent system on top of CVS might be
> useful, I think we can take a few more basic steps. We could, for
> instance, make:
> 
> 
> 	o	-Werror a requirement for patches?
> 

Ahh, -Werror has been my god in rewriting the typesystem.

I use it to pinpoint exactly which code needs to be updated to deal
with a given new type code's structure.  Usually, if i just remove the
routines (create_set_type became make_set_type, etc. There was a mix
before, i just made them consistent), or remove a macro, i just get
various *warnings* about unreferenced this, or that. 
Since the lookup_pointer_type now returns a struct pointer_type *, for
instance, i pinpointed all code that would need to change by compiling
-Werror, since you'll get a conversion warning.
Things like that.
It's easier than trying to grep.

The only problem with requiring -Werror free, or so i've heard,  is
you might end up doing things like what i've done
temporarily in some places during the typesystem rewrite.  I add
explicit cases, particularly where a routine declares a
struct type * at the top somewhere, and uses it to store 18 different
kinds of types in it.  Since the various routines now return the
correct structure for their type (lookup_pointer_type returns a struct
pointer_type), and struct type isn't yet the base of all the types,
i'd get incompatible type assignment, when i know it's okay.

Of course, i'm doing it on purpose, and mark them all with a /*TYPEFIX
Explicit cast check*/ comment, to remember to check them later. Some of them
are correct, of course, since struct type will eventually be the base
for all the types. It's not now so i can still compile gdb while i'm
making type changes, changing over one type code at a time to the new
type structures, etc.

But like I said, I've heard that -Werror promotes this type of behavior elsewhere.


> 	o	a gdbstyle.sh script (a bit like the ari)
> 		script I have that checks things like
> 		indentation and stuff like (free vs xfree)
> 
> The other one is a way of better tracking patches.  At present, in the
> end, it is still me using my mailbox for manual processing.
> 
> 
> 	Andrew

-- 
"I don't like the sound of my phone ringing so I put my phone
inside my fish tank.  I can't hear it, but every time I get a
call I see the fish go like this <<<>>><<>><<<<.  I go down to
the pet store -- "Gimme another ten guppies, I got a lotta calls
yesterday."
"-Steven Wright