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

["Gregory Berkolaiko" <Gregory.Berkolaiko@xxxxxxxxxxxx>]

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

> Gregory: I implemented this

good.  now I can criticize it :)

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

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.

G.