[freeciv-ai] Re: patch/rfc: goto and move/attack test wrapping

["Per I. Mathisen" <per@xxxxxxxxxxx>]

On Mon, 25 Nov 2002, Ross W. Wetmore wrote:
> >> I'm tempted to think you got it reversed.
> >> The wrapper should check and assign the dest_xy values.
> >> This should be the execute with minimal dest_xy sanity check.
> >
> >The way I'm heading, goto destination is setat the same time as you set
> >your mission by calling ai_unit_new_role(punit, role, x, y). I used to
> >have whether we have goto or not decided by a macro called valid_goto(),
> >but I think this should be dependent on our role instead. So certain roles
> >always have a valid goto, and others don't.
>
> I still think you have it reversed, and the fact that you need to pass
> the goto values to have them set back where they started from only confirms
> it :-).

I still don't understand what you mean.

You have basically two different uses for goto:
 - carrying out complex multi-turn missions
   (for example diplomating a city far away or transporting a nuke
    somewhere on a sub)
 - carrying out fast, one-turn or less tasks
   (for example bribing a unit that gets in our way, or hitting a
    transport you find en route)

You want to do the latter without losing sight of the former.

> >That is where I started out. I turned out, however, that the code never
> >used this extra information, and when I tried to make it use it, the
> >benefits were negligible and the resulting code ugly, since all the checks
> >had to be made while the result variable was still guaranteed to be valid
> >and in the right scope. Instead, explicit checks like punit->moves_left>
> >0 or same_pos(x1, y1, x2, y2) were much better and allow better code flow.
>
> If you only need to check GR_DIED now, then you only need to check it
> where you were coding. I don't see the problem.
>
> It is the case when someone else wants to check the other conditions that
> you are blocking with this change.
>
> The final condition truncation is thus both unnecessary for you and
> extremely poor practice as it assumes you know what others want.

I find the GR_* mess completely useless, and if I get my way with
do_unit_goto() it will be rewritten to use callback functions instead of
these inane return values.

Can you please give me examples of how GR_* return values can possibly be
useful? And don't give examples that can easily be solved (better!) with
(punit->moves_left > 0) or same_pos().

> >> is the error log missing???
> >
> >Not really. I don't want to add it before some pathological cases of it
> >are removed, so that users aren't spammed with lots of stupid LOG_ERROR
> >messages.
>
> So, you are saying it is missing. Prefix the comment withTODO: to say so
> and stop inane people like me from wondering if the code is corrupted :-).

Ok.

> >> There is something wrong with this. The lower level function should
> >> *never* destroy state. Push the wrapper consitions down to the
> >> appropriate level.
> >
> >I don't understand. No state is destroyed. It is just that
> >can_unit_move_to_tile() checks if we are idle, since we must be idle to
> >move. But since we want to know if we _could_ move _if_ we were idle, we
> >fake it.
>
> Then the lower level function is one of Jason and Raimar's compound
> obscenities that should be written as two simple parts. The base
> function does the task without checking for spurious conditions. This
> is what you call here. A wrapper function checks for spurious conditions
> before calling the core functionality for those that like that behaviour.
>
> Making every high level code function update and restore its data state
> just to run someone's poorly coded operation is a mistake. Push the
> state changes down to the level where they belong, which in this case
> is nowhere if you recode the called routine appropriately. When you
> run across this, fix the underlying code problem, not the new code here.

Hmmm. I doubt either Jason or Raimar have touched this code. In any case,
there is already a function that can be used for this deep in unit.c.
However, it is

enum unit_move_result test_unit_move_to_tile(Unit_Type_id type,
                                             struct player *unit_owner,
                                             enum unit_activity activity,
                                             bool connecting, int src_x,
                                             int src_y, int dest_x,
                                             int dest_y, bool igzoc)

which is kinda messy. But since it is used only twice (each), I suppose it
will do. For the attack case we really should use both the above and
can_unit_attack_tile().

  - Per