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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sun, Sep 02, 2001 at 08:37:27PM +0200, Gaute B Strokkenes wrote:
> On Sun, 2 Sep 2001, hawk@xxxxxxxxxxxxxxxxxxxxxxx wrote:
> > 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.
> 
> What's your point?

I run indent over all sections I change before I commit a patch.

> >> >> -           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
> 
> You can go and put a comment there if you feel the need.  I was
> planing to eliminate the problem entirely in the immediate future; in
> the meantime I wanted to make it clear to patch readers and the like
> why this is needed.

Ok.

> >> >> -  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==/!/.
> 
> I know that.  There are very few places that use "0 == strcmp".  Some
> do use comparison, but they have the 0 on the right.

==0 is also ok. But !strcmp looses some of the meaning for me.

> >> > 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.
> 
> Yes.  However, MAPSTEP, normalize_map_pos() and the _iterate macros
> are in the same layer of the code.  So there is no harm in writing
> each of them in the most natural and efficient way, but there is
> certainly a performance penalty associated with not doing so.

This depend how may MAPSTEP instances are there outside map.h.

> >> 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.
> 
> For a patch this small I certainly would not, though I probably would
> have produced them separately if I was to start from scratch.
> Remember that most of this came from unifying my fixes and those from
> Ross.  At this point it would be a lot more work, for very little
> gain, to separate them.  Besides, as a maintainer I know what
> stylistic changes are and are not likely to be appreciated by a
> maintainer, namely myself.

I still think that separating them would make it easier to see what
really has changed. I think over 50% of your patch are formatting
changes.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "I was dead ... but I'm better now."
    -- Capitain Sheridan in Babylon 5