From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Berlin To: Andrew Cagney Cc: Eli Zaretskii , Jim Blandy , gdb-patches@sources.redhat.com Subject: Re: Patch review process Date: Wed, 13 Jun 2001 22:26:00 -0000 Message-id: <874rtjiqe1.fsf@cgsoftware.com> References: <3B279F7E.4050405@cygnus.com> X-SW-Source: 2001-06/msg00275.html Andrew Cagney 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