[Freeciv-Dev] Re: Core is_tiles_adjacent

[Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx>]

 --- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote: 
> On Thu, Oct 04, 2001 at 03:36:46PM -0700, Paul Zastoupil wrote:
> > > Assertion failed: is_tiles_adjacent(punit->x, punit->y, x, y), file
> > unittools.c, line 3097
> > 
> > #0  0xff21b638 in _libc_kill () from /usr/lib/libc.so.1
> > #1  0xff1b5644 in abort () from /usr/lib/libc.so.1
> > #2  0xff1b58e8 in _assert () from /usr/lib/libc.so.1
> > #3  0x264ec in goto_route_execute (punit=0x198868) at
> unittools.c:3103
> > #4  0x35488 in do_unit_goto (punit=0x198868,
> restriction=GOTO_MOVE_ANY, 
> >     trigger_special_ability=1) at gotohand.c:1079
> > #5  0x21514 in update_unit_activity (punit=0x198868) at
> unittools.c:993
> > #6  0x2046c in update_unit_activities (pplayer=0xb) at
> unittools.c:595
> > #7  0x40338 in update_player_activities (pplayer=0x1171b8) at
> plrhand.c:175
> > #8  0x153e4 in end_turn () at srv_main.c:420
> > #9  0x176ac in main_loop () at srv_main.c:1728
> > #10 0x17bd8 in srv_main () at srv_main.c:1970
> > #11 0x11ef8 in main (argc=0, argv=0xffbefa24) at civserver.c:147
> 
> Mhh
> 
> There was a test 
> 
> -    if (is_tiles_adjacent(punit->x, punit->y, x, y)) {
> 
> in the code before the patch. Gregory removed it in the patch. I added
> it as an assert (praise defensive programming). So either I applied
> the patch wrong or the patch was itself wrong (which is also my
> mistake because I haven't tested it). Gregory?

The assert does not belong in there.  I removed
  if ( is_tiles_adjacent(punit->x, punit->y, x, y) )
not because they are always adjacent, but because there is this same
check in handle_unit_move_request which follows in 2 lines and it gives a
proper debug message and returns a proper fail value (actually not
proper, it returns 0) and it is caught and digested later in the code.

And I did explain it in my submission email.

So this is the case of editing the patch without understanding it.  I am
not saying you have to understand it to commit it, but you do have to
understand it to change it (except formatting).

If I saw the edited patch before committing I would have pointed out the
mistake.  So I think you should either commit patches unchanged or
consult the submitter about the changes (again other than formatting).

Concerning formatting, we should have a consistent policy and everybody
should try to follow it, so there is less work for the developers.

Best wishes,
G.

P.S. Raimar, this is not to say that you are doing bad job.  I think you
are doing great job and the project is alive 80% through your activity
(see CVS log).  I also think that there should be more committing
developers (i.e. those who are ready to review and commit the patches by
_other_ submitters), partly to give Raimar more time to work on (the very
promising) agents front.  There should also be policy of checking
developers' own patches, something like "the developer cannot commit a
patch that he made".
P.P.S. Now I mixed lots of things in one email.  Should I split it?

____________________________________________________________
Do You Yahoo!?
Get your free @yahoo.co.uk address at http://mail.yahoo.co.uk
or your free @yahoo.ie address at http://mail.yahoo.ie