[Freeciv-Dev] Re: Core is_tiles_adjacent

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Oct 05, 2001 at 04:54:55PM +0100, Gregory Berkolaiko wrote:
>  --- 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.

Shame on me.

> 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).

No. I think that the maintainers have to understand all code they
apply. Or some people from freeciv-dev has to agree that the patch is
good. This is also reason I haven't applied a patch which changed the
spelling of french cities. I just can't verifiy this issue. And at
least on the paper we have more than one maintainer.

> 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.

Since I run all code through indent this is not a problem.

> 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).  

Thanks.

> 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)

I don't like the problem of choosing among you (the freeciv-dev
readers). It is hard to choose. Maybe the older maintainers can give
some advise about "HR".

> , 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".

IMHO there was no violation of this policy yet. Or was it?

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  A supercomputer is a computer running an endless loop in just a second