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

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

On Sat, Dec 28, 2002 at 11:27:00PM +0100, Serge van den Boom wrote:
> On Sun, 22 Dec 2002, Steven Barker wrote:
> > On Sun, Dec 22, 2002 at 08:14:29PM +0100, Serge van den Boom wrote:

> > > 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 compil=
er
> > warning it generated without my patch was (paraphrased) "cast from poin=
ter
> > to integer of a different size".  It seems that in several places, poin=
ters
> > (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.  I=
f 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 li=
ke
> a good idea in itself.

There was one warning about it in src/sc2code/

> I agree with your statement about covering up symptoms, so I'll won't app=
ly
> the patch.

Ok.  Perhaps I'll look into what that code is doing, and see if I can put
together a better fix.

> > > 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 commen=
ts
> > 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.

My patch is exceedingly minimal, as all it does is remove the extra
function declaration (which never should have been put inside another
function to begin with).  Even if it's totally unused code, please consider
applying the patch just to get rid of the silly compiler warning (which
might distract other people from real bugs).

> > > 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 underst=
and
> what you mean.

Yes, I know, C is stupid like that.  My code above actually does it right,
since it casts to an unsigned type (COUNT) before doing the % operator.

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

Well, the current WRAP_VAL function doesn't do any checks, and will have
unexpected results for values outside the normal range.  My equivilant code
above is more consistant (it will always return a value between 0 and w-1).

> > 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 "compariso=
n 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 (CO=
UNT
> > is unsigned).  The other two possible result values are pMS->CurState a=
nd
> > pMS->CurState - (EMPTY_SLOT + 3), which then get compared to PLANET_LAN=
DER
> > (which has the value 0).  This whole mess has the same effect as the co=
de 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 g=
oing
> 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.
=20
> > 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.

I understand what WRAP_VAL is for, and I think it does an ok job of it (may=
be
not the best, as I say above, but certainly good enough).  My point is that
I do not think that WRAP_VAL is what we want at the place my patch changes.
Please look at the code and consider my point.

We call the DisplayLanders function in two cases, when pMS->CurState is equ=
al
to PLANET_LANDER (if we just bought a new lander) and when pMS->CurState is
equal to EMPTY_SLOT + 3 (if we have just sold a lander).

WRAP_VAL is the wrong thing to call.  It happens that WRAP_VAL(pMS-CurState,
EMPTY_SLOT + 3) =3D=3D PLANET_LANDER, when pMS->CurState is EMPTY_SLOT + 3,=
 so the
existing code works, but it's not very clear what we're checking, is it?

Loot at the other places where the value of pMS->CurState is being checked.
Here's one example (from higher up in outfit.c):

		switch (NewState =3D pMS->CurState)
		{
				case PLANET_LANDER:
				case EMPTY_SLOT + 3:
						old_slot_piece =3D pMS->delta_item < GLOBAL_SIS (NumLanders)
										? PLANET_LANDER : (EMPTY_SLOT + 3);
						LastItem =3D MAX_LANDERS - 1;
						break;
				...[other cases]...

These are the same cases that get checked later on where my patch is applie=
d.

--=20
Steven Barker                                      st...@bl...
  Artistic ventures highlighted.  Rob a museum.
Get my GnuPG public key at: http://www.blckknght.org/publickey.asc
Fingerprint: 272A 3EC8 52CE F22B F745  775E 5292 F743 EBD5 936B