[Freeciv-Dev] Re: (PR#7279) Macro optimizations

["Jason Short" <jshort@xxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=7279 >

Ross Wetmore wrote:
> Arnstein Lindgard wrote:

> But if you decide that you want to manage storage in other ways, you can,
> for example, define whatever global blocks you need and reference them
> instead with no actual MACENV(my_env) statements placed in the code at
> all.

Only so long as you don't invoce the macro twice in one place.  The hmap()
example given previously is one such case.  In this case you can't use
globals or things will be undefined.  And the only way you'll know is if
your compiler is smart enough to give you a warning about it.

>> may not neccessarily get with inlining. And we have to add the "my_env"
>> argument to all the macro functions?
>>
>> bool normalize_map_pos(int *x, int *y)    would become
>> normalize_map_pos(ENV, X, Y)
>
> Right. But one could also start to use the env argument for more useful
> environment than just local storage.
>
> bool normalize_map_pos(struct map *map, int *x, int *y)
> normalize_map_pos(PMAP, X, Y)
>
> These are functions that use instance map data rather than getting things
> like map.xsize from a global. Thus one can have several different maps or
> worlds and do appropriate topology transformations in any of them in the
> same game.
>
> If the leading argument is an instance object rather than just a block
> of storage, it starts to provide a much more powerful paradigm.

You can't have it both ways.  Your map parameter can't be both a global
and provide local variables.  For instance you can't do

  hmap(pmap, x, y) = hmap(pmap, x1, y1) + foo();

because again you're using the same local storage for both macro calls.

>> What about type checking?
>
> The simplest technique for type checking is the obvious one. Write all
> your
> code as functions. Then provide macro forms for those that have an obvious
> performance benefit with a switch to run the optimized code or by calling
> the
> function. You then can compile for typechecking, or for performance if you
> chose mutual exclusivity, or allow the function to coexist for cases where
> you want to also be able to do a function call.
>
>    #ifdef _OPT_MACFN_
>    #define  macfn(args)   (<code>)
>    #else
>    #define  macfn(args)   _macfn(args)  // the real function
>    #endif

In this case you end up with twice the code.  This does not help
maintainability.

This isn't an argument against the use of macros, just an argument that
they shouldn't be duplicated with functions.  Whatever type-checking
benefits you may get are more than offset by having two separate pieces of
code that must be both maintained and debugged in parallel.

>> On Tue, 20 Jan 2004 20:18:15 -0800 Jason Short wrote:
>>
>>>map_pos_to_index, as written, needs temp variables.  If written in macro
>>>form (without any change to the interface) these temp variables must be
>>>made global.  This doesn't work since multiple calls to this function
>>>can be made within one sequence.  For instance
>>>
>>>   hmap(x, y) = hmap(x1, y1) + foo(); /* hmap calls map_pos_to_index */
>>>
>>>is not valid C if map_pos_to_index isn't coded properly.
>>
>> That's just the situation I ran into with undefined behaviour, so
>> I begin to see the use for inlining, at least with map_pos_to_index().
>
> Of course, one can just code it properly as an alternative to inline, or
> break the code into segments that do not interact.

Yep.  That's what the original map_pos_to_index macro did.  It was quite a
bit slower than the inline form because it couldn't make use of temporary
variables.

> It is interesting though that the error never arose in the corecleanup
> codebase and all the ingredients are being used. I would still like to see
> your code example and macro definitions. Dumping out the preprocessor code
> might also reveal just what the problem is as one can then look at the raw
> "C" to see what you macro actually did. Jason's explanation just doesn't
> make sense without something more like mismatched brackets or the like.

No, it's not interesting at all.  You rewrote all the calls to
map_pos_to_index to conform to the calling restriction that you
arbitrarily imposed without documentation.  Hence when the macro was
copied over verbatim into CVS code it didn't work, and I had to figure out
why.

> One really should document the best practices and reasons for them as a
> summary of this sort of discussion.

True.  But this is difficult without any agreement.

jason