Worm_data access

[draqo]

This patch changes all references from struct worm_data to string_data, when stackp could be possibly >0. Accessing worm_data in this situation is wrong, because worm_data holds valid values only when stackp==0. I also changed move_reasons - added find_origin calls (that's why it is in the same patch) and more asserts.

[draqo]

Look for comment from 01/23/07 00:19:53 in ticket 148 for regression description of this ticket.

[arend]

Thanks for the patch.

Some comments:

• You change a few places where the code is known to be run at stackp == 0 only, e.g. everything in move_reasons.c. I have no strong opinions on these, but I would rather leave that code as it is. (In particular when we need to access worm data anyway, I would rather add an gg_assert(stackp==0) than change only some of the accesses to worm data.)
• The experimental code in owl.c dealing with GAIN etc. (whether one part of a dragon gets captured in the fight) on purpose wants to use the original worm sizes etc. - at least in my understanding.

I will attach a small patch extracting the parts that I do think are fixing problem, and will leave the rest for others to judge.

Your aggressive adding of assertions in move_reasons.c seems good to me.

(P.S.: About the copyright assignments: just e-mail me or Gunnar and we will forward it to Dan, as he probably doesn't know you, I don't have your e-mail address I believe.)

[arend]

Small extract of worm_data.diff with one additional fix in owl_mark_worms

[draqo]

Replying to arend:

Thanks for the patch.

• You change a few places where the code is known to be run at stackp == 0 only, e.g. everything in move_reasons.c. I have no strong opinions on these, but I would rather leave that code as it is. (In particular when we need to access worm data anyway, I would rather add an gg_assert(stackp==0) than change only some of the accesses to worm data.)

Yes, you are right. Asserts are better. Really, I didn't check all places if something should be changed or not, I did it rather mechanical way :)

(P.S.: About the copyright assignments: just e-mail me or Gunnar and we will forward it to Dan, as he probably doesn't know you, I don't have your e-mail address I believe.)

I don't know your email too, it's probably somewhere in the doc or here but, it would be nice if you post me it here. My email is draqo@…

[draqo]

Replying to draqo:

I don't know your email too, it's probably somewhere in the doc or here but, it would be nice if you post me it here. My email is draqo@…

And what should I email? There's some form to download?

[draqo]

Replying to arend:

• You change a few places where the code is known to be run at stackp == 0 only, e.g. everything in move_reasons.c.

I didn't add any access to worm_data in move_reasons - everywhere there is worm_data[pos].origin. I only added this in helpers.c in call to add_either_move. And by the way, this function isn't called when stackp==0, there is trymove before the call - so move_reasons functions are not always called at stackp==0 (this isn't good situation but changing this I'm leaving to someone else - or for me, when I get there in analyze :)

I have no strong opinions on these, but I would rather leave that code as it is. (In particular when we need to access worm data anyway, I would rather add an gg_assert(stackp==0) than change only some of the accesses to worm data.)

I think that accesing string_data is better, because we check more asserts (like in find_origin) and the cost is only one more read (to string_number table). Even is something is run at stackp==0 now, maybe future implementations change it (and not every functions that should be called at 0 depth are called properly - eg. situation with move_reasons). If somebody have time, this could be overlooked and changed, but this would some work and as for now I think that accessing string_data is good (I changed back this in some places where this is clear that stackp would be 0 - this will show in a new patch that I'll post soon).

• The experimental code in owl.c dealing with GAIN etc. (whether one part of a dragon gets captured in the fight) on purpose wants to use the original worm sizes etc. - at least in my understanding.

You're right. This will be also considered in corrected patch.

[draqo]

Replying to draqo:

I only added this in helpers.c in call to add_either_move. And by the way, this function isn't called when stackp==0, there is trymove before the call - so move_reasons functions are not always called at stackp==0 (this isn't good situation but changing this I'm leaving to someone else - or for me, when I get there in analyze :)

Ok. This situation isn't a good example, because there is a check if board[worma] and board[wormb] have stones on them. But maybe somewhere else it's wrong...

[draqo]

I added the improved patch with all corrections made. Now it should change access only in places where it is desirable. I modified this patch partly manually and maybe this is the reason why HTML preview for this patch is unavailable (but 'patch' program is working corectly with this patch).

[draqo]

propagate_worm2 correction - previous ver was working because function was called only with origin of worm as str