[Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Aug 15, 2001 at 11:58:09AM -0400, Jason Dorje Short wrote:
> Raimar Falke wrote:
> > 
> > On Wed, Aug 15, 2001 at 03:28:26AM -0400, Jason Dorje Short wrote:
> > > Trent Piepho wrote:
> > > >
> > > > On Wed, 15 Aug 2001, Jason Dorje Short wrote:
> > > > > This patch removes the magic numbers and expressions associated with 
> > > > > the
> > > > > DIR_D[XY] macros, collecting all such implementation-dependent code 
> > > > > into
> > > > > map.[ch].
> 
> > > > > Additions to map.[ch]:
> > > > >  - macros dir_reverse(dir) and dir_is_cardinal(dir) are created
> > > >
> > > > Since these are macros, convention is to give them names in all caps.
> > >
> > > They are macros, but act just like functions (and could be made so in
> > > the future).
> > 
> > I would like to second Trent here. There are macros and this property
> > should be visible by the name. You may also define both: a macro and a
> > function.
> 
> How come this convention isn't held by, say, the iteration macros? 

The code isn't perfect. If you want to do it... At least for new code
this schema can be adopted.

> Sorry to be argumentative here, but this seems counter-intuitive to me -
> and also makes the code look uglier IMO, which you cite as a reason for
> another change.
> 
> As for the names being too long, DIR_REVERSE seems to be much less
> confusing than DIR_REV.

An extra comment would be helpfull. You may also say what makes a
cardinal direction so special.

> > > > >  - the directional names used for logging ("N") are defined globally
> > > >
> > > > Wouldn't a function be better than a global variable?
> > 
> > I also thought about this when I read the patch.
> 
> > > These strings are only used in debugging output, and should not be
> > > translated.  I don't see any other advantage of a function - you
> > > might as well use a function in place of DIR_DX and DIR_DY.  Am I
> > > missing something?
> > 
> > Compare
> >   freelog(LOG_NORMAL, "direction of next step is %s",DIR_NAMES[dir]);
> > with
> >   freelog(LOG_NORMAL, "direction of next step is %s",get_dir_name(dir));
> > 
> > At least for me the second one looks nicer. And you can also put extra
> > checks in the function. So I would propose something like this:
> 
> Okay - although I don't think it really looks nicer, having extra checks
> in the function is nice.

Can you change 
  char* dir_get_name(int dir);
to
  const char* dir_get_name(int dir);
?

> I've currently put the function prototype in map.h next to the
> definition of DIR_DX/DIR_DY, even though all other function prototypes
> are collected up above.  This makes sense to me, but is it the correct
> thing to do?

Normally the order of things in a header file is only of minor
importance. And AFAIK there no standard (like for example: first
#defines, then macros, then enums, then structs, then variables then
functions) for this ordering in freeciv.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Only one human captain has ever survived battle with the Minbari
  fleet. He is behind me. You are in front of me. If you value your 
  lives, be somewhere else."
    -- Ambassador Delenn, "Severed Dreams," Babylon 5