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

[Gaute B Strokkenes <gs234@xxxxxxxxx>]

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.

>> -        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.

>>  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.

>> +/* 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.  

> 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.

> 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.

> 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.

-- 
Big Gaute                               http://www.srcf.ucam.org/~gs234/
UH-OH!!  We're out of AUTOMOBILE PARTS and RUBBER GOODS!