Re: [Sc2-devel] Look Ma, no compiler warnings!

[<st...@bl...>]

On Sun, Dec 22, 2002 at 08:14:29PM +0100, Serge van den Boom wrote:

> Well, they might eliminate some warnings (not all though), but I think
> these warnings warrant some closer attention.

I agree with your caution.  I'll try to justify my patches as best I can.
I'm also curious about other compiler warnings you mention.  Those four
patches fix all the ones I see on my system (tested with both gcc 2.95.4
and 3.2.2pre2).

> Why expand the size of MEM_HANDLE? Is that size actually needed?
> Wouldn't a few type casts be better? I'd like to hear your rationale.

Well, the code in question looks pretty shady in many ways.  The compiler
warning it generated without my patch was (paraphrased) "cast from pointer
to integer of a different size".  It seems that in several places, pointers
(typically char *s) get cast to MEM_HANDLEs.  To be truely correct for all
archetectures something better will have to be done to rework that code, but
my fix will make it a lot saner for 32 bit systems in the mean time.  If you
think that covering up the symptom of this issue is worse than leaving it,
don't apply the patch.

> Putting an external function declaration inside a function is not good
> practice. The right solution would be to include the correct .h file.

I think the function the declaration is inside of is unused (the comments
claim it's "unimplemented").  The warning was for a redeclaration of the
function, as GetUNICODEKey is defined elsewhere in the file.  This is
definately the right fix for it, unless we want to remove the
"unimplemented" functions entirely.

> Your WRAP_VAL patch is downright wrong. (take a look at the actual
> definition of the macro).

I did.  Its basically a mod operator?  Its common cases are equivelant to
(((COUNT)(v)) % (d)), right?  (I assume it's not ever used where
v < -d or v >=3D 2*d.)

In any case, I'm pretty sure that WRAP_VAL is the wrong thing to use in
this case.  The warning the compiler gave was something like "comparison is
always false due to data type", refering to the fact that one of the
comparisons in the expansion of WRAP_VAL(pMS->CurState, EMPTY_SLOT + 3),
namely "pMS->CurState < 0", is always false, since  CurState's type (COUNT
is unsigned).  The other two possible result values are pMS->CurState and
pMS->CurState - (EMPTY_SLOT + 3), which then get compared to PLANET_LANDER
(which has the value 0).  This whole mess has the same effect as the code I
supstituted, testing against pMS-CurState against PLANET_LANDER (0) and
EMPTY_SLOT + 3 directly.

It's also a lot easier to understand, since those are precisely the two
states for which DisplayLanders should be called (when a new lander was
purchased, and when one was sold, respectively).

> The NumVisits patch is correct. SBYTE is good enough though. Applied in t=
hat
> form.

Ah, yes, I think I had it that way at one point.  I went back and forth a
few times.  SBYTE is just as good for the sizes of values we're dealing
with.  A bigger type might be better if we ever intend to change the game
to cover more situations than were in the original SC2, but I think that
it is both unlikely to happen any time soon and that it would require
major rewrites anyway to get rid of other similar limits.

--=20
Steven Barker                                      st...@bl...
  "Consider a spherical bear, in simple harmonic motion..."
  		-- Professor in the UCB physics department
Get my GnuPG public key at: http://www.blckknght.org/publickey.asc
Fingerprint: 272A 3EC8 52CE F22B F745  775E 5292 F743 EBD5 936B