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

[Gaute B Strokkenes <gs234@xxxxxxxxx>]

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?

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

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

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

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

-- 
Big Gaute                               http://www.srcf.ucam.org/~gs234/
..  the HIGHWAY is made out of LIME JELLO and my HONDA
 is a barbequed OYSTER!  Yum!