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

["Arnstein Lindgard" <a-l@xxxxxxx>]

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

> >>On Sun, 4 Jan 2004 11:30:36 +0100 Raimar Falke wrote:
> >>
> >>>>+#define is_city_hilited(pcity)                                           
> >>>> \
> >>>>+(map_get_tile((pcity)->x, (pcity)->y)->client.hilite == HILITE_CITY)     
> >>>> \
> >>>>+
> >>>
> >>>It was my error to not report earlier but such macros should really be
> >>>functions.

On Sun, 04 Jan 2004 09:57:02 -0500 Ross Wetmore wrote:

> The current status of the inline function vs macro discussion and rules
> are that macros are the accepted and correct way to handle simple cases.
> For more complex ones one should use a full functional implementation.

> Inline raises issues with both compatibility support and value (since
> it is an optional vs fixed directive to the compiler) and thus should
> never be used in Freeciv code.
> 
> If Raimar wants this trivial macro to be coded as a function, he should
> probably do the test comparisons to see how much of a performance impact
> this raises.
> 
> IMHO, this is exactly the sort of trivial expression that macros are
> designed to implement in a way that can be easily maintained and
> understood. Changing this to a function is not yet needed to deal with
> any complexity issues.

I agree with Ross, but not because of speed concerns. When I see a
call to a simple function, I expect the reason for the function not
being a macro is that something variable needs to be computed at that
point in the code flow. Ie, the programmer is saying that we need a
computed return value that is relevant at this point in time, for the
given parameters.

I consider the above macro to be a pure boolean expression because we
know that the tile pointer isn't changing. Then again, I would've put
a tile pointer in the city struct in the first place, avoiding
another call.

Although I've become used to it from Freeciv hacking, I would
previously never have written something like this:

{
  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?

If you do NOT expect that, then why are you instructing the CPU to
backup it's registers, recalculate focusunit, and restore the
registers, all in the middle of a bool. There is no macro alternative
here, but it's got something to do with what you expect from
functions. This also has speed impact in core functions, but again
this is not my point.

I think it's slightly annoying that nedit displays macros in uniform
color, but I value explicitness in the logic of the source code far
more than multicolor.

I don't know whether this discussion has something to do with OO
programmers emulating their normal style in assembly, er, I mean C,
but I think Ross makes the best case.


Arnstein