[Freeciv-Dev] Re: PATCH: cleanup in fill_tile_sprite_array (PR#1016)

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:
> 
> On Wed, Oct 17, 2001 at 04:08:42PM -0400, Jason Dorje Short wrote:
> > Raimar Falke wrote:
> > >
> > > On Tue, Oct 16, 2001 at 03:14:13PM -0400, Jason Dorje Short wrote:
> > > > New patch attached.

> > < +      if(tspecial_near[DIR8_NORTH] & S_RIVER ||
> > ttype_near[DIR8_NORTH]==T_RIVER)
> > < +     *sprs++ = sprites.tx.river_outlet[DIR4_NORTH];
> > < +      if(tspecial_near[DIR8_WEST] & S_RIVER ||
> > ttype_near[DIR8_WEST]==T_RIVER)
> > < +     *sprs++ = sprites.tx.river_outlet[DIR4_WEST];
> > < +      if(tspecial_near[DIR8_SOUTH] & S_RIVER ||
> > ttype_near[DIR8_SOUTH]==T_RIVER)
> > < +     *sprs++ = sprites.tx.river_outlet[DIR4_SOUTH];
> > < +      if(tspecial_near[DIR8_EAST] & S_RIVER ||
> > ttype_near[DIR8_EAST]==T_RIVER)
> > < +     *sprs++ = sprites.tx.river_outlet[DIR4_EAST];
> > ---
> > > +      int dir8;
> > > +
> > > +      for (dir8 = 0; dir8 < 8; dir8++) {
> > > +     if (!DIR_IS_CARDINAL(dir8)) {
> > > +       continue;
> > > +     }
> > > +     if (tspecial_near[dir8] & S_RIVER || ttype_near[dir8] == T_RIVER) {
> > > +       *sprs++ = sprites.tx.river_outlet[dir8_to_dir4(dir8)];
> > > +     }
> > > +      }
> >
> > You cannot simply substitute the for loop here.  You should not safely
> > assume that the ordering will be the same (N, W, S, E).  In fact the
> > current ordering is N, W, E, S, but may be changed in the future.
> > Unless the code using this data is changed as well, this must keep the
> > fixed ordering.
> 
> Since the 4 river_outlets are disjunct (don't share a pixel) this
> isn't a problem.

I don't quite understand this.  Are you saying the ordering of elements
in sprs doesn't matter?  If so, that could perhaps allow a lot more
cleanups of this function.

I'm not entirely sure that a SAFE_MAPSTEP macro is a good idea.  It
makes things a little cleaner, but if it's there people will be inclined
to use it and that's usually not a good idea :-).  It should be ok,
though.  Oh, one other thought: nearest_real_pos strongly implies that
it will create only a real position; it doesn't say it'll be a normal
position.  However, it says in the comments for the function that it
will normalize.  Should the calling code assume this, or should it call
normalize_map_pos after calling nearest_real_pos?  Up to now we have
been assuming the former.

Aside from that, everything looks good.  Your other changes are all just
cleanups of what I did.

jason