[crossfire] safe/common item stack/inventory iteration?

[Mark Wedel]

  Looking at a crash on metalforge, and it is in a loop like:

     for (tmp = op->inv; tmp != NULL; tmp = next) {
         next = tmp->below;
         { do some work }
     }

  Now this construct is used to cover the case where the current object (tmp) 
goes away - we have a pointer to the next object.  Such operations are not that 
uncommon, as the {do some work} block is more likely to destroy tmp.

  However, in this case, it appears that next was destroyed, so after {do some 
work} completed, tmp became next, and next (tmp->below) pointed to garbage, so 
the iteration after that crashed.  In fact, in that second iteration, all of tmp 
was probably garbage, but perhaps didn't crash on that because the checks done 
(in this case, in the plugin.c file) probably didn't match - tmp->type and 
tmp->subtype did not match what was being looked for.

  At some level, handling this becomes difficult - it doesn't make sense to have 
a next_next pointer, as there could probably be cases where the next two objects 
get destoryed.  The easiest thing in this case is to probably store something 
like next_count, and see if that differs (suggesting the item was destroyed), 
and if so, just bail out of the loop.  This could perhaps cause some small 
inconsistencies (loop didn't do all the processing it should), but that is 
probably better than a crash.

  However, I know there are loops like that all throughout the code, so I wonder 
if it would be better to macroize that in some way.  The correct code block is 
something like:

     next_count=op->inv?op->inv->count:0
     for (tmp = op->inv; tmp != NULL; tmp = next) {
	/* May want to set tmp to null before the break, so that if
	 * anything after the for loop tries to use it, it won't be
	 * able to
	 */
	if (tmp->count != next_count) break;
         next = tmp->below;
	/* if next is NULL, this won't process again in any case */
	if (next) next_count=next->count;
         { do some work }
     }

  I wonder if that could be put in a macro.  Well, I know it could, but the 
problem is really that { at the end of the for line - ideally you want to be 
able to do something like:

   FOR_BELOW_OB(op, tmp) {
	{do some work }
   }

  With the macro taking care of declaring declaring the variables to handle the 
checking of destroyed status, etc.  But you really want that opening brace to be 
in the code segment with the loop, simply because most editors key on that for 
indentation and other factors.  A possibility is to have two macros, so 
something like:

   FOR_BELOW_OB(op, tmp, next, tmp_tag) {
      	CHECK_BELOW_OB_STATUS(op, tmp, next, tmp_tag);
	{do some work }
   }

   So that at least the braces line up.  Thoughts?  It would seem good to try to 
make this standard, since it seems this is fairly common, and a crash seen one 
place could occur almost anyplace.