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

[Serge v. d. B. <sv...@st...>]

On Sun, 22 Dec 2002, Steven Barker wrote:
> 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).
They're mainly about negative shifts. We've actually got a patch lying here
ready for that, but it will change the save game format, so we'll keep that
for later, and apply it together with a number of other changes that affect
the save game format later.

> > 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.
Pointers cast to integers? I haven't noticed those. That doesn't sound like
a good idea in itself.
I agree with your statement about covering up symptoms, so I'll won'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.
'unimplemented' meant what it says. I don't know why that remark is still
there, maybe it's not completely implemented yet. Anyhow, the input code is
being revised, this should be taken care of in time.
For now, I'll leave it as it is.

> > 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?
Actually, % returns a negative value when the first argument is negative.
It's something every programming language does differently. But I understand
what you mean.

> (I assume it's not ever used where v < -d or v >= 2*d.)
Certainly that should be checked instead of assumed.

> 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.
There's a reason for using defines for constants. (If this is not evident to
you, you better ask in some C or general programming newsgroup; I'm not going
to spend time on this). Throwing them out is not acceptable.
Using something like WRAP_VAL makes perfect sense; this macro is meant to
apply wrap-around on a value, and that's exactly what it's doing here.

> 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).
I disagree. WRAP_VAL handles wrap-around. Once you know this, the code is
much easier to parse for a human.


Greetings,

Serge