[crossfire] useful macros in crossfire

[Alex Schultz]

Mark Wedel wrote:
> Generally speaking, just because the code is repeated, it isn't
> necessarily
> bad if the the expanded code isn't very long, is clear to understand, and is 
> unlikely to need global replacement because of some other change in the the code.
>   
However, IMHO it is a problem when functionally identical pieces of code
are written many different ways and can be confusing to people who are
new to the code.

>   OTOH, I'm pretty use to the code, so when I see most of those operations, it 
> is pretty clear to me what it is doing.
>   
I'm also pretty used to the code, however the thing about the same
operation being written 50 different ways does bother me a bit, plus I
recall that back when I first was looking at the codebase about a year
and a half ago, those things sometimes took me a little while to figure
out. Also, one potentially confusing thing sometimes, is that with
inventories one has to go up through the stack of objects, however with
objects on the ground one must search down, which while being something
I'm quite used to now, is not something that is not necessarily clear to
less familiar programmers.

>   I'm more inclined to accept the GET_.. ones than the TRANSVERSE ones.  They 
> seem cleaner - maybe it is just me, but having something like:
>
>    TRANSVERSE_INV() {
>    }
>
>   seem a bit more confusing to me, as it is actually hiding the operation from 
> you, which may be relevant depending on what you do in that inner loop.  That 
> could perhaps be improved by calling them something like FOREACH, which better 
> describes what it is doing.
>   
Hmm, FOREACH would probably be better. I was just using transverse
because that's an accurate term that I usually describe these operations
as, however FOREACH is probably better for this, partially because it's
more familiar to programmers in certain languages.
Well, though it hides some operation anyone who can understand the loops
as now, easily could easily find out how the macros work, especially so
if they're also documented.  Also it leaves less room for mistakes; It
can be rather frustrating to accidentally make a off by one error in the
loop, or transverse the wrong direction.

>   But also, I think that several of the TRANSVERSE macros as is will have 
> problems, as I know there is code like:
>
>   for (tmp=ob->inv; tmp && next; tmp=next ) {
> 	next = tmp->below
>   }
>
>   Because depending on operations, you can't rely on tmp->below to be accurate 
> after doing remove_ob or insert_ob calls.  This is true for both inventory and 
> map loops.  So that now needs another set of macros to take that third object 
> pointer.
>   
That is indeed an issue, though one could always make
"SAFE_TRANSVERSE_*" versions which  use an extra tmp variable to  store
the next one.
Also, here we have a perfect demonstration the above mentioned issue off
by one errors, it just so happens that your example here will stop one
short of the end of the inventory, because next will be NULL when one
away from the bottom of the inventory which will stop the loop ;)

>   To me, going through the code and updating all those code snippets would be 
> near the bottom of the list of things to do, given the fairly long TODO list 
> that there currently is (and given that the this doesn't effectively change the 
> operation of any of the code, it just changes how it looks).  But making the 
> macros available and used in new code may not be a bad thing.
Agreed.

Alex Schultz