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

[Jason Dorje Short <jshort@xxxxxxxxxxxxx>]

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].
> 
> I see that it doesn't change the directions, so there should be fewer
> possibilities for bugs.  But if you changed them, then any missing magic bits
> would be easier to find.  Of course, maybe you already did that when you
> looked for things to change.

The directions remain the same (for now)...the patch fixes things up
without any controversy (the behavior should be identical).

Yes, I have tried things out with different directions (changing the
macros as necessary).  Everything seems ok.

> > 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.  Perhaps
> shorter names..

They are macros, but act just like functions (and could be made so in
the future).  The dir_ prefix was recommended - reverse and is_cardinal
seem like the most logical names.

> >  - the directional names used for logging ("N") are defined globally
> 
> Wouldn't a function be better than a global variable?

To make it easier to internationalize?  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?

> I think this is a pretty good idea.  The directions are really a mess they way
> they are now.

Well, there's another mess here.  The client defines its own set of
cardinal directions, using an enumeration and the names DIR_NORTH, etc. 
There will inevitably be confusion.  I made a comment about this in my
own enumeration.

jason