[Freeciv-Dev] Re: (PR#3928) Convert client to use PF

["Raimar Falke" <rf13@xxxxxxxxxxxxxxxxx>]

On Fri, Apr 04, 2003 at 01:42:34PM -0800, Gregory Berkolaiko wrote:
> Raimar,
> 
> Quoting Raimar Falke <rf13@xxxxxxxxxxxxxxxxx>:
> 
> > The attached patch removes large parts of client/goto.c. Remaining are
> > the waypoint, packet and drawn handling.
> > 
> > Open issues are:
> >  - the network interface doesn't support dangerous tiles
> >  - the path execution at the server doesn't support dangerous tiles
> >  - the network interface is quite weird (for example it divides the
> >  path into 20-positions chunks while a chunk could holds 1500/2=750
> >  positions)

> >  - I had to add ../common/pqueue.o by hand. The reason for this is
> >  unknown.

Link order was the reason. Fixed.

> > 
> > Except because of the last point the attached patch should be applied.
> 
> What, right away?
> How about making it better first? ;)
> 
> 1. The patch doesn't apply.  Since the chunk that fails is huge, I
> will ask you to update the patch.

Done.

> 2. It is poorly documented.  I remind you of this line in the style
> guide: " do _not_ introduce a new function without some sort of
> comment."  Please also comment the structures you are introducing
> and anything else that is non-trivial.

Done. I like this one:
/********************************************************************** 
  Remove the last part.
***********************************************************************/
static void remove_last_part(void)

:P

> > Gregory: I implemented this
> 
> good.  now I can criticize it :)

Yeah.

> >   struct pf_path *pft_concat(struct pf_path *dest_path,
> >                              const struct pf_path *src_path);
> > 
> > But it isn't a true concat because it removes the duplicated and
> > overlapping opsitions.
> 

> 3. It removes just one duplicating position, as far as I see.  Where
> is my mistake?

One position for one call of pft_concat. Multiple removed positions
for multiple calls.

> 4. What's wrong with realloc and then memcpy?  With them you can do
> both cases as one and get rid of variables like i and j (why are
> they not in their scopes?).

While the memcpy form is more compact I think it is harder to
read. Changed however.

> 5. It's a bit hard to tell just looking at the diff, but it seems
> you have committed the greatest possible travesty: recalculating map
> each time the mouse is moved.  If it is so, you have to repent, fast
> for three days and fix it by using pf_get_path on the old map.

You have looked correct. Fixed.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Transported to a surreal landscape, a young girl kills the first woman
  she meets and then teams up with three complete strangers to kill again."
    -- TV listing for the Wizard of Oz in the Marin Independent Journal

cc6.diff.gz
Description: cc6.diff.gz