Complete.Org: Mailing Lists: Archives: freeciv-dev: August 2001:
[Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1
Home

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

[Top] [All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
To: rf13@xxxxxxxxxxxxxxxxxxxxxx
Cc: Jason Dorje Short <jshort@xxxxxxxxxxxxx>, freeciv-dev@xxxxxxxxxxx
Subject: [Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1
From: Gaute B Strokkenes <gs234@xxxxxxxxx>
Date: Thu, 16 Aug 2001 02:02:41 +0200

On Wed, 15 Aug 2001, hawk@xxxxxxxxxxxxxxxxxxxxxxx wrote:
> On Wed, Aug 15, 2001 at 11:58:09AM -0400, Jason Dorje Short wrote:
>> Raimar Falke wrote:
>> > 
>> > 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.

Macros that do not behave precisely like functions (e.g. they do not
evaluate each argument precisely once) should definitely be all caps.
Other than that, it's a toss up.  There are macros that violate this
rule all over Freeciv, but I hope to eradicate them over time.

In this case, since DIR_IS_CARDINAL() needs to be all caps, it makes
sense to do it for the other one as well.

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

Normally I do not approve of this sort of functions since it only
makes the situation look more complicated than it really is.  However,
if the purpose of the function is debugging, then I suppose it is
worthwhile.

Incidentally, this patch is just the sort of cleanup thing that I want
to see.  It's small, it's focused and it follows the rules to the
letter.  I will be applying it shortly; my only reason for not doing
so immediately is that it touches some parts that I'm also changing.

-- 
Big Gaute                               http://www.srcf.ucam.org/~gs234/
CONGRATULATIONS!  Now should I make thinly veiled comments about
 DIGNITY, self-esteem and finding TRUE FUN in your RIGHT VENTRICLE??


[Prev in Thread] Current Thread [Next in Thread]