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

["rwetmore@xxxxxxxxxxxx" <rwetmore@xxxxxxxxxxxx>]

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


I tried to figure out what the actual problem was that you ran into and
Jason is trying hard to bill as some critical issue with map_to_index().

Although the actual code and error messages were not published as asked,
I am assuming that the concern is with the lintish warning something like
"operation on global values may be undefined" that you get with -Wall
when you try to assign global values in both lvalue and rvalue context
in the same statement. If it is not, my apologies. Please provide the
actual details and I will try again.

As usual it appears to be a noise problem introduced by poor coding
practices used elsewhere rather than anything of technical concern with
the macro he replaced using this argument. In effect he has masked a
poor coding practice with this fix rather than fixing the poor code,
and in the process degraded performance plus introducing a number of
other undesirable impacts.


The warning is correct, the values left behind in storage used for
temporaries can result from either of the rvalue or lvalue tree-node of
operation paths being executed last. This would be of concern if you
tried to use the global values after this statement and before they were
redefined, but of course does not change the correctness of the code in
the statement itself. The storage *is* technically undefined after the
statement is executed.


The problem is not with map_to_index(), but an artifact of the versatility
of macros in doing string manipulations on "C" code as well as emulating
function calls. It is the "C" code produced elsewhere that is the issue,
and there are any number of ways to write similar poor code, using
map_to_index() being part of only one of them and therefore not a general
fix.

The hmap() macro is written as a function, but of course its usage as an
lvalue in the code line below only works because it is being (ab)used as
a string handling technique to generate "C" code. hmap() as used here could
never be done as a function either called or inline, and is thus an example
of rather poor coding practices.

In cases like this, the hamp() macro should at a minimum be uppercased to
show that it uses pure macro features and is not a function replacement.
By carrying out function calls, assignments or other operations as a side
effect of generating an lvalue, one is generating poor code that often
results in lint warnings like in the current example. The coder of the
macro should work hard to avoid such things, and document those that remain.
Users should check and adhere to the documentation and respect standard
"C" coding rules and conventions in writing their user code.


Like a lot of lint warnings, one usually fixes these by correcting code,
rearranging the offending code or adding technically unnecessary code
*locally* as a last resort to turn off the lint noise.

It is certainly not the case that inline is required to fix this, and in
fact, besides obscuring the real problem (cosmetic noise from a poor code
practice), it will have significant performance penalties because the
optimization is now only a hint and code in many places may no longer be
optimized. In fact map_to_index() has now become an expensive function
call replacing what would have been a trivial operation that was turned
into an API largely for convenience (even though there is also a beneficial
degree of merit for optimization from using standardized template coding
practices).

Macros can handle convenience APIs with no performance impact, while
inline functions because of their inherent limitations causing them to
fallback to out-of-line calls cannot.

Use of inline has also restricted the compilation environment, since the
original code would run on any vanilla "C" compiler, and this is no longer
the case.


So Jason's *fix* to map_to_index() has produced the *benefits* of

1)  removing a cosmetic lint warning
2)  masking a poor coding practice
3)  decreasing overall program performance
4)  confusing the debugging process
5)  restricting the compilation environment
6)  breaking a long established coding standard in favour of arbitrary
     user practices

Doesn't seem like a particularly good fix given that there are any number
of other workarounds to solve the noise issue with no adverse impacts,
a pure issue, which of course was not of technical concern in the first place.

He should backout the map_to_index() fix and take another crack at this
to get it right :-).


It would also help if explanations given actually presented the real
technical issue and problem, rather than giving some sort of interesting
pseudo jargon and a lot of inuendo to divert the attention from the real
problem into a reason to promote a pet plan for a cosmetic code purge.
It is always better to force people to work things through fully and use
valid technical reasons to justify or evaluate proposed fixes.


Cheers,
RossW
=====

rwetmore@xxxxxxxxxxxx wrote:
> <URL: http://rt.freeciv.org/Ticket/Display.html?id=7279 >
> 
> Arnstein Lindgard wrote:
> 
>><URL: http://rt.freeciv.org/Ticket/Display.html?id=7279 >
>>
>>On Tue, 20 Jan 2004 17:44:09 -0800 rwetmore@xxxxxxxxxxxx wrote:
[...]
>>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.
> 
> 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.
[...]
>>Arnstein
> 
> Cheers,
> RossW
> =====