On Wed, Jun 28, 2006 at 05:42:30AM -0700, Jimmy Huang wrote: > main.c line 437, where I use space-tab combo for the > indent, and the rest of the surrounding code is spaces > only? That's one of many examples. I shouldn't have to point them all out, because you should have examined the diff closely yourself. The patch acceptance policy that I follow is in the file STYLE. But since you asked, I will point them out ... I'll review your patch below ... which makes this a long mail. > I personally like spaces only, but someone told me space-tab combo > was better. Regardless of what is better, you must follow the style in the file you are editing. > When editing the code in emacs, I can't see the difference, unless I > move my cursor around and around seeing it jump more than 1 space.. That's easy to fix. Customise your emacs. For instance, have a look at these methods on the Emacs Wiki ... http://www.emacswiki.org/cgi-bin/wiki/ShowWhiteSpace Especially note the "Show Tabs Using a Face (Color)" section, which I've just tried on my emacs and it works great. If you try this, make sure you add a hook section for c-mode. > A cursory glance, and everything else seems to line up > fine. It doesn't line up, when the patch is displayed ... therefore I presume you didn't review the patch bands yourself. Be honest ... did you? Check every line of a patch you are proposing. It shouldn't be difficult, and it's a great way to spot problems. -- Reviewing your patch ... (while it is possible to review it in the darcs mail form, and some of the problems are obvious even there, here it is changed to unified diff format, by doing a "darcs get" of my repo, "darcs apply" of your patch, then "darcs diff -u --last=1" to generate the diff, then quoted into my mail buffer ...) You wrote: > Wed Jun 28 18:37:21 EST 2006 jimmyhua73 at yahoo.com > * robotd-war-decs-bugfix.dpatch Priority: high. Problem: the patch name doesn't meet the STYLE requirements, in that it is not in user-visible NEWS style. See "Darcs - Patch Name and Long Comment" in the STYLE file. Threat: makes it harder for us to generate the NEWS file, makes it harder for people to understand the purpose of the patch by reference to just the patch name. > * robotd/data.c fixed spelling on comments. Priority: low. Problem: the long comment doesn't follow ChangeLog standards, because the colon is missing, and function names are missing. See "Darcs - Patch Name and Long Comment" in the STYLE file. Threat: makes it harder for us to find a cause of a later problem. (otherwise the long comment is fantastic, well done). > diff -rN -u old-netrek-server/Vanilla/robotd/data.c new-netrek-server/Vanilla/robotd/data.c > --- old-netrek-server/Vanilla/robotd/data.c 2006-06-29 14:28:12.000000000 +1000 > +++ new-netrek-server/Vanilla/robotd/data.c 2006-06-29 14:28:12.000000000 +1000 > @@ -45,7 +45,7 @@ > int override = 0; > int doreserved = 0; > int lastm; > -int delay; /* delay for decaring war */ > +int delay; /* delay for declaring war */ > int rdelay; /* delay for refitting */ > int mapmode = 1; > int namemode = 1; Fine. > diff -rN -u old-netrek-server/Vanilla/robotd/decide.c new-netrek-server/Vanilla/robotd/decide.c > --- old-netrek-server/Vanilla/robotd/decide.c 2006-06-29 14:28:12.000000000 +1000 > +++ new-netrek-server/Vanilla/robotd/decide.c 2006-06-29 14:28:12.000000000 +1000 > @@ -34,6 +34,7 @@ > if(!inl){ > if((_state.warteam < 0 || _state.total_wenemies == 0) && > _state.total_enemies > 0){ > + /* beginning of game, no enemies */ > declare_intents(); > return; > } Fine. > @@ -41,6 +42,7 @@ > else if(status->tourn){ > if((_state.warteam < 0 || _state.total_wenemies == 0) && > _state.total_enemies > 0){ > + /* beginning of game, no enemies */ > declare_intents(); > return; > } Fine. > diff -rN -u old-netrek-server/Vanilla/robotd/main.c new-netrek-server/Vanilla/robotd/main.c > --- old-netrek-server/Vanilla/robotd/main.c 2006-06-29 14:28:12.000000000 +1000 > +++ new-netrek-server/Vanilla/robotd/main.c 2006-06-29 14:28:12.000000000 +1000 > @@ -49,6 +49,7 @@ > char *defaultsFile = NULL; > register i; > int rwatch=0; > + static int switchedteams=0; > char password[64]; > int timer2 = 0, tryudp=1; > char **_argv = argv; Priority: medium. Problem: change of indenting style from existing style in function. The lines above and below the added line use tabs for indenting. The new line does not. So when it is displayed in a patch it shows up as a change of style. Threat: makes it harder to review the patch. > @@ -400,6 +401,7 @@ > > mprintf("team ...\n"); > if(!teamRequest(team, s_type)){ > + switchedteams = 1; > mprintf("team or ship rejected.\n"); > > /* if base destroyed */ Fine. > @@ -412,8 +414,9 @@ > > showteams(); > do { > - if(teamRequest(team, s_type)) > + if(teamRequest(team, s_type)) { > break; > + } > if(read_stdin) > process_stdin(); > readFromServer(1); Fine. > @@ -431,6 +434,7 @@ > timer2 = 0; > _state.team = team; > if (!teamRequest(team, s_type)){ > + switchedteams=1; > mfprintf(stderr, "team or ship rejected.\n"); > showteams(); > } Fine. Although this also looks wrong during patch review, since "switchedteams" starts in a different column to the mfprintf call. This is because the mfprintf line uses spaces, and the switchedteams line uses tabs. The added line is consistent with surrounding code, but it looks like a change of style. It may have been more correct to fix the indenting of the mfprintf and showteams lines. > @@ -459,6 +463,11 @@ > sendUdpReq(COMM_UDP); > } > > + if (switchedteams) { > + switchedteams=0; > + declare_intents(); > + } > + > input(); > } > Fine. > diff -rN -u old-netrek-server/Vanilla/robotd/robot.c new-netrek-server/Vanilla/robotd/robot.c > --- old-netrek-server/Vanilla/robotd/robot.c 2006-06-29 14:28:12.000000000 +1000 > +++ new-netrek-server/Vanilla/robotd/robot.c 2006-06-29 14:28:12.000000000 +1000 > @@ -2002,6 +2002,9 @@ > strcpy(_state.ignore_e, ignore_e); > > bzero(&_timers, sizeof(_timers)); > + /* This declare_intents() only gets run when you are dead > + or when someone sends a reset command to the robot. > + If you are dead, declare_intents does nothing */ > declare_intents(); > req_cloak_off("reset"); > return 1; Priority: medium. Problem: new lines are using a different indentation style, specifically the first of the added lines uses spaces and the second two use tabs. > @@ -2065,9 +2068,7 @@ > > sendUpdatePacket((int)(_state.timer_delay_ms * 100000.)); > init_comm(); > - /* > - declare_intents(); > - */ > + > } > > ignore_edefault(s) Fine. Although you've added a blank line ... I would have only removed the lines, not added a blank one. If you hadn't reviewed your change you probably would not have noticed. (The point again being that you should review your change, and you should make the change as small as it can be, provided it remains consistent. When reviewers see this kind of change, they can't help but think you didn't review it.) > diff -rN -u old-netrek-server/Vanilla/robotd/update_players.c new-netrek-server/Vanilla/robotd/update_players.c > --- old-netrek-server/Vanilla/robotd/update_players.c 2006-06-29 14:28:12.000000000 +1000 > +++ new-netrek-server/Vanilla/robotd/update_players.c 2006-06-29 14:28:12.000000000 +1000 > @@ -1024,13 +1024,25 @@ > } > > /* war & peace. Done initially */ > -/* BUGS */ > > +/* Fixed so, when you first enter the game 10 seconds in, the robot > + will declare war on the top 2 teams with most players. > + If the robot is a 3rd team scummer, it works out quite nicely. > + > + Will also declare war correctly when forced to join a different team, > + say during timercide. During Timercide, winners keep their old > + war declarations (which is the right thing to do), as they don't > + get forced to join a different team. > + > + Doesn't work when you lose t-mode and people join back in > + on different teams, as the function to check for this condition doesn't > + exist, yet. */ > declare_intents() > { > register i; > register struct player *j; > - int teams[16], maxt = 0; > + int teams[16], maxt=0, maxcount=0; > + int maxt2=0, maxcount2=0; > int newhostile, pl=0; > extern char *team_to_string(); > Priority: medium. Problem: new line added changes indentation style (maxt2 and maxcount2). You've used spaces instead of tabs. Oh, make sure you use a monospaced font for reading this mail, otherwise you won't see it the way I do ... which is that the "t" of teams[16] lines up with the "=" of "maxt2=0". > @@ -1053,21 +1065,36 @@ > pl++; > } > > + if (!pl) return; > + > if(pl){ > - for(i=1; i< 9; i *= 2) > - if(teams[i] > maxt) > - maxt = i; > + for(i=1; i< 9; i *= 2) { > + if(teams[i] > maxcount) { > + maxt = i; > + maxcount = teams[i]; > + } > + } Priority: medium. Problem: new "if" inside the "for" isn't indented properly? Actually, it is, but only because tabs are used instead of spaces. The "for" uses spaces. Your new code uses tabs. So it looks wrong. So actually it's a change of indenting style again. > mprintf("max team = %s\n", team_to_string(maxt)); > + > + for(i=1; i< 9; i *= 2) { > + if( (teams[i] > maxcount2) && (i != maxt) ) { > + maxt2 = i; > + maxcount2 = teams[i]; > + } > + } And again. > + mprintf("max team2 = %s\n", team_to_string(maxt2)); > + > + And two blank lines? > } > - /* > - else > - maxt = -1; > - */ > > /* peace */ > for(i=1; i< 9; i *= 2){ > if(i == maxt) continue; > - if((i & me->p_hostile) && !(i & me->p_swar)){ > + > + if (i==maxt2) continue; > + > + /* Is hostile to AND not at war with */ > + if((i & me->p_hostile) && !(i & me->p_swar)) { > mprintf("declaring peace with %s\n", team_to_string(i)); > newhostile ^= i; > } And again. > @@ -1075,12 +1102,21 @@ > _state.warteam = maxt; > > /* war */ > - if(maxt != -1){ > - if(!((maxt & me->p_swar) || (maxt & me->p_hostile))){ > + if(maxt != 0 ) { > + if(!((maxt & me->p_swar) && !(maxt & me->p_hostile))) { > mprintf("declare war with %s\n", team_to_string(maxt)); > - newhostile ^= maxt; > + newhostile |= maxt; > } > } And again, the mprintf looks improperly indented. Oh, and an extra space after "maxt != 0" > + > + /* being hostile to more teams is better than less teams */ > + if(maxt2 != 0 ) { > + if(!((maxt2 & me->p_swar) && !(maxt2 & me->p_hostile))) { > + mprintf("declare war with %s\n", team_to_string(maxt2)); > + newhostile |= maxt2; > + } > + } > + > if(newhostile != me->p_hostile) > sendWarReq(newhostile); > } > This change is what really caused me to look harder at your patch in the first place ... only because there's an extra space after the conditional expression in the if. -- Finally something about technical review ... when I got your patch I had the choice of either fixing the problems you had with it, accepting it as is, or rejecting it. A review such as the above is meant to find the problems, but isn't meant to tell you how to fix them. The review is intended to be positive, so that you can do better next time. This is learned behaviour for me, from being part of a software engineering group that valued quality. We had ISO900x certification and some really cool procedures to increase quality of our code. A technical review would normally happen in a conference room with paper copies of the code, or nowadays with a shared editor with multiple laptops and a projector or two ... and again, we aren't allowed to fix the code, only to highlight the problems. -- James Cameron mailto:quozl at us.netrek.org http://quozl.netrek.org/ -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 189 bytes Desc: Digital signature Url : http://mailman.us.netrek.org/pipermail/netrek-dev/attachments/20060629/dadb0a75/attachment.pgp