[Freeciv-Dev] Re: cartesian_adjacent_iterate in map.h

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Feb 06, 2002 at 05:22:27AM -0500, Jason Short wrote:
> Raimar Falke wrote:
> 
> > On Tue, Feb 05, 2002 at 10:20:53PM +0000, Vasco Alexandre Da Silva Costa 
> > wrote:
> > 
> >>Anyone objects if i change that to:
> >>
> >>#define cartesian_adjacent_iterate(x, y, IAC_x, IAC_y) \
> >>{                                                      \
> >>  int IAC_i;                                           \
> >>  int IAC_x, IAC_y;                                    \
> >>  CHECK_MAP_POS(x, y);                                 \
> >>  for (IAC_i = 0; IAC_i < 4; IAC_i++) {                \
> >>    IAC_x = CAR_DIR_DX[IAC_i];                             \
> >>    IAC_y = CAR_DIR_DY[IAC_i];                             \
> >>                                                           \
> >>    if (!normalize_map_pos(&IAC_x, &IAC_y))            \
> >>      continue;
> >>
> >>#define cartesian_adjacent_iterate_end                 \
> >>  }                                                    \
> >>}
> >>
> > 
> > No. But splint has an option where it warns about constructs like
> > this:
> >  for(...)
> >  {
> >    for(...)
> >    {
> >      continue or break
> >    }
> >  }
> > 
> > with "are you sure that you didn't mean to continue/break the outer
> > loop"? I don't think it is a good idea to change freeciv to get no
> > such warning at all but in the above case in can be avoided without
> > cost.
> 
> In this case, there is more danger in using an if() statement than there 
> is in the continue.  If you just have
> 
>    if (normalize_map_pos(&IAC_X, &IAC_y)) {
> 
> in the macro, then a foolish/careless/clever user can insert an } else { 
> into the code that uses the macro, giving (probably) bad results.

This may also be a feature.

> This isn't really much of a concern, but it's there nonetheless...
> 
> 
> As to the macro, I'm all in favor of simplifying it.  I'd do it as
> 
> #define cartesian_adjacent_iterate(x, y, itr_x, itr_y)
> {
>    adjc_dir_iterate(x, y, itr_x, itr_y, _IAC_dir) {
>      if (!DIR_IS_CARDINAL(_IAC_dir))
>        continue;
> 
> but I think macro wrapping like this is frowned upon :-).

But this has a clear performance penalty. And the CPU can't also
predict the branches.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Sit, disk, sit. Good boy. Now spin up. Very good. Here's a netscape
  cookie for you. Fetch me some data. Come on, you can do it. No, not that
  data. Bad disk. Bad." 
    -- Calle Dybedahl, alt.sysadmin.recovery