[Freeciv-Dev] Re: [PATCH] Corecleanup_07Part2 has been put in incoming

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sun, Sep 02, 2001 at 06:35:05PM +0200, Gaute B Strokkenes wrote:
> On Sat, 1 Sep 2001, hawk@xxxxxxxxxxxxxxxxxxxxxxx wrote:
> > On Sat, Sep 01, 2001 at 05:41:06AM +0200, Gaute B Strokkenes wrote:
> > 
> > There is a lot of noise (the "_end-}" thing, s/return(a)/return a/
> > thing, other formatting changes,...) in this patch. Is this
> > necessary?
> > 
> >> -  }
> >> -  city_map_checked_iterate_end;
> >> +  } city_map_checked_iterate_end;
> > 
> > Although it is a bit ugly indent moves the *_end on a new line.
> 
> indent doesn't know about _iterate macros.  Virtually all modern code
> in Freeciv is like this and it has been the preferred form for some
> time; I see no reason to stop now.

I will not rearrange the _end macro by hand after an indent run over a
code section.

> >> -      ok++;
> >> -      break;              /* out of adjc_iterate */
> >> -    }
> >> +  if (map_get_terrain(x2, y2) == T_OCEAN
> >> +      && is_my_zoc(unit_owner(punit), x2, y2)) {
> >> +    ok++;
> >> +    goto OK;
> >> +
> >> +    /* FIXME: The above used to be "break; out of adjc_iterate",
> >> +    but this is incorrect since the current adjc_iterate() is
> >> +    implemented as two nested loops.  */
> > 
> > You fixed it. So the comment is useless.
> 
> Que?  adjc_iterate() is still implemented as two nested loops.  The
> comment explains why the obvious will not work.
> 
> I'd bet that if I didn't put it there you would be asking me why I
> obfuscated the code like this.

This is possible. However we should try to impress this into the mind
of every programmer and not put into the code somewhere. A better
place for this comment is map.h

> >>  static void find_city_beach( struct city *pc, struct unit *punit,
> >>  int *x, int *y)
> >> -  int search_dist = real_map_distance( pc->x, pc->y, punit->x, punit->y );
> >> +  int search_dist = real_map_distance(pc->x, pc->y, punit->x, punit->y) - 
> >> 1;
> >> +  
> >> -  square_iterate(punit->x, punit->y, search_dist - 1, xx, yy) {
> >> +  square_iterate(punit->x, punit->y, search_dist, xx, yy) {
> > 
> > Well this is equivalent but why did you changed it?
> 
> Because the current form is an obfuscation.  It is sort of like
> initializing a variable called "what_we_are_looking_for" and then
> XORing it with 0xDEADBEEF before using it, if you catch my drift.
> 
> >> -  for (tt = T_FIRST; tt < T_COUNT; tt++) {
> >> -    if (0 == strcmp (tile_types[tt].terrain_name, name)) {
> >> +  for (tt = T_FIRST; tt < T_COUNT; tt++)
> >> +    if (!strcmp (tile_types[tt].terrain_name, name))
> > 
> > I like the old form more.
> 
> I don't.  Also, it is more consistent with the rest of the code (in
> Freeciv, and the rest of the world, I suspect).  Came to think of it,
> I should remove the space after "strcmp" as well.

It is not about the { but the s/0==/!/.

> >> +/* We're using a trick here.  The straightforward way to write
> >> +IS_VALID_POS() would be as
> >> +
> >> +  0 <= (y) && (y) < map.ysize
> >> +
> >> +however, since we know that map.ysize is always positive and we know
> >> +that the bit-pattern of a negative signed int is the same as the
> >> +bit-pattern of a very large number when interpreted as an unsigned
> >> +int, we can do away with the extra check.  
> >> +*/
> >> +  
> >> +#define IS_VALID_POS(x,y) \
> >> +  (((unsigned) (y)) < ((unsigned) map.ysize))
> >> +
> >> +#define is_real_tile(x,y) IS_VALID_POS(x,y)
> >> +
> >> +#define IS_PROP_TILE(x,y)                          \
> >> +  (IS_VALID_POS((x), (y))                          \
> >> +   && (((unsigned) (y)) < ((unsigned) map.ysize))  )
> > 
> > I don't see a difference between IS_VALID_POS and IS_PROP_TILE.
> 
> Oops.  Cut and paste.  It should be IS_SANE_POS() as promised, anyway.
> 
> >> +#define MAPSTEP(x,y,x1,y1,dir)     \
> >> +  do {                               \
> >> +    (x) = (x1) + DIR_DX[(dir)];      \
> >> +    (y) = (y1) + DIR_DY[(dir)];      \
> >> + /* if ((x) == -1)               */  \
> >> + /*   (x) = map.xsize-1;       */  \
> >> + /* else if ((x) == map.xsize) */  \
> >> + /*   (x) = 0;                   */  \
> >> +    (x) = map_adjust_x((x));       \
> >> +  } while (0)                      
> > 
> > IMHO MAPSTEP should be a method which does:
> > int mapstep(int old_x,int old_y,int *new_x,int *new_y,int dir)
> > {
> >     *new_x = old_x + DIR_DX[dir];
> >     *new_y = old_y + DIR_DY[dir];
> >     return normalize_map_pos(new_x,new_y);
> > }
> > 
> > With such a method we get everything which has to do with position
> > creating and checking in one method and place.
> 
> I don't see much gain in doing so.

IMHO we should try to encapsulate common code patterns. This put all
this in one place and will also decrease the amout the programmer has
to write (the hopefully this will also decrease the chance that the
programmer will do an error). The whole _iterate macros are about
this.

> > The method resembles common code structures.
> 
> What are you trying to say?  Using pseudo-OO language and words like
> "method" instead of "function" doesn't make it easier to work out what
> you're trying to say.

I'm sorry. I use method and function interchangeable. This has nothing
to do with OO.

> > Overall you should really remove the noise of the patch.
> 
> It's not noise, it's beautiful music...most of this is fallout from
> resolving CVS conflicts and the like so the code has been recently
> changed anyway.  As far as I'm concerned code formatting cleanup is
> one of the less glamourous, but still useful things that a maintainer
> is supposed to do.
> 
> I suppose I could apply it separately, but I don't see what the added
> work would gain anyone.

Please do it separately. It will make it easier for freeciv-dev to
proofread it. Come on we (the maintainers) would also demand this from
a patch author.

> > I'm not sure but it looks to me that some of loops you fixed are
> > allready fixed in the cvs.
> 
> You're hallucinating. 8-) I made the patch within minutes of sending
> the message.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Any sufficiently advanced technology is indistinguishable from magic."
    -- Arthur C. Clarke