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

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

At 12:35 PM 02/11/24 +0000, Per I. Mathisen wrote:
>
>On Sat, 23 Nov 2002, Ross W. Wetmore wrote:
>>
>/**************************************************************************
>> >+This will eventually become the ferry-enabled goto. For now, it just
>> >+wraps ai_unit_goto()
>>
>> 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 set at 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 :-).

>+**************************************************************************/
>> >+bool ai_unit_gothere(struct unit *punit)
>>
>> Return a valid move status, bool hides too much.
>
>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.

>> >+/* log error on same_pos with punit->x|y */
>>
>> 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 with TODO: to say so
and stop inane people like me from wondering if the code is corrupted :-).

>> >+bool ai_unit_can_move(struct unit *punit, int x, int y)
>> >+{
>> >+enum unit_activity activity = punit->activity;
>> >+bool can_move;
>> >+bool igzoc = unit_flag(punit, F_IGZOC);
>> >+
>> >+punit->activity = ACTIVITY_IDLE;
>> >+can_move = can_unit_move_to_tile(punit, x, y, igzoc);
>> >+punit->activity = activity;
>>
>> 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.

>  - Per

Cheers,
RossW
=====