[Freeciv-Dev] Re: (PR#7229) Re: Re: Freeciv commit: ali: Add macro is_city_hilited().

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

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

There are a couple points that the following digression brings up
which are perhaps useful in designing or critiquing future APIs.
Ideally one should document such rationales in the style guide as
suggested practices.

Arnstein Lindgard wrote:
> <URL: http://rt.freeciv.org/Ticket/Display.html?id=7229 >
> 
> On Sat, 10 Jan 2004 12:52:26 -0800 Jason Short wrote:
> 
>>>{
>>>  if (same_pos(punit->x,
>>>               punit->y,
>>>               get_unit_in_focus()->x,    /* L1 */
>>>               get_unit_in_focus()->y)) {
>>>    ...
>>>  }
>>>}
>>>
>>>Instead I would've written:
>>>{
>>>  struct unit *focusunit = get_unit_in_focus();
>>>  if (same_pos(punit->x,
>>>               punit->y,
>>>               focusunit->x,    /* L1 */
>>>               focusunit->y)) {
>>>    ...
>>>  }
>>>}
>>>
>>>Do you expect a piece of silicon to generate an interrupt precisely
>>>at L1 which may change punit_focus via a GTK timer?
>>
>>This is wrong on many levels.
>>
>>1.  If the value ever were changed at L1, the first piece of code would be
>>wrong since it would use one unit's X value and another unit's Y value.
> 
> Which is my point exactly. We are producing C code that accounts for
> the possibility of an interrupt at L1, even though we know that it
> will not modify punit_focus AND it would have been all bollocks if it
> did. I see style 1 (excessive usage of function calls) as suitable if
> we for some reason needed an updated focus unit (return value of the
> function) at that point in time. But we neither need nor want it.

The second form is actually preferable from many different standpoints
even if multithreading is not currently allowed in Freeciv. But note,
if a macro is used for get_unit_in_focus() in the first case, it is
almost certain that the compiler will optimize out the double computation.
Relying on this is bad form, but such considerations reduce the technical
impact.

A point that is being missed is that it is the same_pos() API that is
actually poorly coded. In an O-O environment, not only would one have an
entry with double int arguments, but also one that took Point arguments.
Thus the call would be same_pos(punit->pos, get_unit_in_focus()->pos).

A better fix that case 2 is to define the same_pos() API(s) in ways that
preclude or at lest provide obvious alternatives to code use that is
inherently flawed.

>>2.  Freeciv clients are single-threaded, so there will never be an
>>interrupt to change the value at L1.
> 
> We could have one.

Exactly, but more to the point, if one has a choice in designing code
correctly in ways that are proper in a wide range of environments, rather
than building in all sorts of poorly documented no-nos and gotchas to
be avoided, and there is really no performance or other reason not to do
so, then insisting on using bad coding habits is pure and simply bad coding.

One of the biggest problems in Freeciv and many other projects as well is
the continual use of excuses like "this case will never happen" as a reason
to support inclusion of bad code. Sooner or later, the code changes and
the reason does happen, often because of the overwhelming number of nested
and contradictory layers of such unnecessary restrictions.

[...]
>>3.  The second form is faster, but negligibly so (except in code that
>>"needs" optimization).  So it's really just a matter of style.  I much
>>prefer the second form.

This is another case where cummulative rationalization sooner or later
sinks a project. If one way is better, then justification of the other
way by "it doesn't hurt" never makes a lot of sense. Only a stronger
valid technical reason should trump a preferred reason.

> It is also a matter of straightforwardness. I would require a reason
> for recalculating something at a point where recalculation is
> pointless (not the other way around).
> 
> Sometimes faster code could also be a sign that you did something
> proper, even if blood-optimizing currently is politically incorrect
> because of Moore's Law, or because Brandon will yell at us.

This is actually a very worthwhile statement. Doing something faster or
simpler usually means less room for fault and abuse. Following previously
thought out practices that have these characteristics makes for consistent
robust code more easily maintained and less subject to crashing because
it has fewer flaws that might be opened up down the road.

> The second form also opens the possibility of an interrupt modifying
> punit_focus. We are not stupid and won't actually do that. The
> previous sentence does not really reverse the requirement for giving
> reasons.

That is why it might be best to fix the same_pos() API to take Points
and thus only a single reference to either punit or get_unit_in_focus(()
arguments is required rather than one for each 2-D coordinate which
then opens up the possibility for poor coders to abuse.

> It's fair to mention that I also prefere Ross' line of argumentation
> because the consequences of those opinons accomodate my personal
> style, which is entirely functional. But that's not an argument for
> or against anything.

Thanks. But it is more a matter that I think one should not change the
programming style of a written program without long and careful thought.

Freeciv is a "C" program and until a major version change mandates a new
programming language, maintainers should spend effort updating CVS with
the backlog of existing patches rather than starting a new project and
code purge to render all those patches obsolete because they no longer
apply to CVS.

I actually prefer O-O coding style over functional given a choice. But
I am a good enough programmer to adapt to the current style and maintain
consistency, rather than engaging in continual software rewrites as
different preferred styles compete for non-technical dominance.

In some cases careful thought says make changes, but a few maintainers
seem to spend inordinate amounts of time doing cosmetic rather than
technical improvements. These should only happen once in a long while in
a single effort that introduces only a momentary spike in outstanding
patch compatibility. Much of the cosmetic change can happen over time
though as real technical changes conforming to agreed standards replace
existing code.

The key is to develop and maintain consistent longterm standards.

No inline functions is one of the current longstanding standards.

> Arnstein

Cheers,
RossW
=====