next competition (67)

**Pete Mack** · March 24, 2009, 07:45

DaJaband Warnings and BUGS

I did an OSX build and got zillions of dangerous warnings, some of which are certain bugs.

First and foremost:
There's a disastrous misuse of the global variable, spellswitch.
I have no idea what this thing is or how it works. It appears to be a massive goto across multiple source files. GET RID OF IT, or you won't ever be able to maintain your code.

Some of the warnings from dajaband are obtrusive and potentially dangerous:
multiple embedded /* in comment, in defines.h
ditto externs.h, line 537

Almost certain bug, otherwise an incredibly dangerous construction:
1947 birth.c: embedded /* in comment

dangerous warnings, probable bug:
birth.c:545: warning: statement with no effect
birth.c:546: warning: statement with no effect

another dangerous warning. Just uncomment it, even though it's a no-op, or use #if 0 construct:
birth.c:1997: embedded /* in comment

Borderline illegal constructs:
cmd0.c:316: warning: implicit declaration of function ‘do_cmd_castluck’
cmd0.c:318: warning: implicit declaration of function ‘do_cmd_castchem’
cmd0.c:320: warning: implicit declaration of function ‘do_cmd_castblack’

BUG!!!!!
cmd1.c:885: warning: control reaches end of non-void function

Dangerous Mystery warning. (I have no idea whether this is a bug or not):
files.c:1166:44: warning: "/*" within comment

Another dangerous mystery:
load.c:1456:26: warning: "/*" within comment
Note: use

Code:

#if 0  /* TODO: flags4 */
code;
#endif

for this kind of disabled code. It is much easier to find after you decide what to do with it. The tag TODO: flags4 allows you to grep for all instances of this future feature. (BTW: Microsoft has (or had) a patent on this, no lie!)

More borderline illegal warnings:
melee1.c:1599: warning: implicit declaration of function ‘dispel_life’
melee1.c:1802: warning: implicit declaration of function ‘dispel_silver’

BUG!!!
melee2.c:461: warning: suggest parentheses around assignment used as truth value

Use #if 0 /* TODO */
melee2.c:897:2: warning: "/*" within comment

Borderline illegal:
melee2.c:2007: warning: implicit declaration of function ‘scare_monsters’

Use:
#if 0 /* TODO Black magic */
object1.c:1019:5: warning: "/*" within comment

Other annoying warnings include:
* unused variable, when the variable is not a function argument.

ALMOST CERTAIN BUG: Multiple redeclarations without use of k_idx:
object2.c:739: warning: unused variable ‘k_idx’
object2.c:744: warning: unused variable ‘k_idx’

BUG AT LINE 1807: (You set both BUG and ANIMAL for all cases.)
spells2.c:1728: warning: unused variable ‘l_ptr’

I HAVE NO IDEA WHAT THIS IS:
spells2.c:4074: warning: suggest parentheses around && within ||

APPARENT BUG: misspelling of descent:
use-obj.c:1218: warning: implicit declaration of function ‘deep_decent’
(Why this warning is so dangerous...., even though it's not technincally a bug.)

BUG: improper (re)declaration of use_charge:
use-obj.c:1417: warning: unused variable ‘use_charge’

x-spell.c:
Warnings are too numerous and confusing to know whether they are bugs or not. NEEDS MAJOR REWRITE.

MULTIPLE BUGS:
xtra1.c:2402: warning: suggest parentheses around && within ||
I do not think it means what you think it means.

**Pete Mack** · March 24, 2009, 07:52

PS:
Rules of thumb:
1. Get rid of compiler warnings, except for a very limited number of acceptably "bogus" warnings. (When in doubt, get rid of the warning.)
1a. Get rid of all warnings in .h files. (These really screw up the build output, since they repeat for every .c file.)
2. Don't use global state variables, unless it's absolutely necessary. spellswitch is a particularly egregious construction.

**will_asher** · March 24, 2009, 13:57

Well, I went in and fixed all that I know how to fix (which was almost all of what you pointed out), and I looked through x-spell.c and cleaned up all the stuff I could find easily (which was several things). I don't know how to find the other stuff. Thanks a lot for that help. (although it'd probably be more fitting in a different thread)

I'm using Dev C++ and it's not giving me all those warnings when I compile. Is there a way I can find this kind of thing using my compiler if it doesn't give me these warnings when I compile or should I use a different compiler?
I had no idea there was anything wrong with having /* comment markers within comment markers. I thought anything within comment markers /* */ had no effect.

I used the spellswitch variable when I needed a variable I could access across different files to do things like tweak the effects of a called function. I ended up using it for several things kindof as a flag, but I kept track of what it does. I thought "why make a bunch of variables when I can use one variable for a bunch of different things?" I can probably get rid of it, but it would be a lot of trouble, and I don't really see why it's that urgent when I kept track of everything it does. (mainly, I have it in a text file, but I also put most of it in a big comment at the end of variable.c, and have just updated that comment so that it explains every use of spellswitch.) You were exactly right, it is a massive goto across multiple source files, and it's quite useful that way. For some of the things I used spellswitch for, it would be hard for me to figure out how it do it without using something like that. Is it really worth the trouble to get rid of it? What would happen if I don't? I mean, the game seems to work fine.

For the competition, I could easily send pav a new zip file with the code I just cleaned up, but when you actually play the game there'd be almost no difference (except for a couple slightly messed up tourist spells). Only a couple of those things were actually bugs, and those few were minor ones (the rest were just sloppy stuff in the code which I don't think would change how the program runs).

**Pete Mack** · March 24, 2009, 15:25

I'm using Dev C++ and it's not giving me all those warnings when I compile. Is there a way I can find this kind of thing using my compiler if it doesn't give me these warnings when I compile or should I use a different compiler?
I had no idea there was anything wrong with having /* comment markers within comment markers. I thought anything within comment markers /* */ had no effect.

There's almost never anything wrong in any one use of embedded /*. It just turns out to be hard to read commented-out code, and if there's a comment embedded in your commented code, the commenting-out will fail. It's a really easy way to introduce bugs, and it's easy to avoid, and easier to read, with unreachable (#if 0) conditional code.

I doubt there's a way to get those warnings with Dev-C++, although the usual way is to add -Wall (or whatever the compiler directive for being persnickety is.)

You can get the warnings if you use MINGW, which is the same compiler as is used on OS X (gcc).

And about that switchspell: it's just very fragile code, and those numbers are impossible to figure out for any casual reader. (The use in curse armor is particularly confusing. I stopped reading in confusion when I saw it.)

The proper way to do this is to add a parameter (usually a flag, #defined value, or enumerated type) to the called function, so only the callee needs to know the actual context in which it is called.

Consider:

Code:

    switchspell = 3;
    foo(x);

...
void foo (int y)
{
   if (switchspell == 3 || switchspell == 2) {...}  /* No context--WTF is 3 or 2?? */
   else ...
}

vs

Code:

    foo(x, true);

...
void foo (int y, bool strong_curse)
{
   if (strong_curse) ...
   else ...
}

**Pete Mack** · March 24, 2009, 15:42

will_asher wrote:

Only a couple of those things were actually bugs, and those few were minor ones (the rest were just sloppy stuff in the code which I don't think would change how the program runs).

"A couple" bugs takes a hell of a long time to find 'in the field' or in testing. Getting rid of as many as possible up front is just a lot easier, and a low-pain way to get them is ... be strict with warnings.

(Even 30 harmless warnings for one bug is a really big payoff in number of bugs. So is 100.)

And the Inigo Montoya code is almost certainly buggy. You write something like:

Code:

if ( cond1 || cond2 && cond3 )

and you often mean

Code:

if ( (cond1 || cond2) && cond3 )

rather than:

Code:

if ( cond1 || (cond2 && cond3) )

which is what the unparenthesized code means.

**MrCba** · March 25, 2009, 15:57

Tourist are fun. This is my favorite Nethack's class

**will_asher** · March 25, 2009, 22:27

DaJAngband tourists are pretty different from Nethack tourists (although I did give them a handy camera flash spell which stuns monsters for a few turns).

I'll send the savefile to Nick tomorrow, looks like it'll be either an assassin or a tourist.

EDIT: Another thing I thought I should mention about the tourist: In DaJAngband, almost all classes have strong pseudo, but the wizard and tourist don't. Of course, they get the identify spell earlier than any other classes. (The tourist gets it even earlier than the wizard).
I playing it right now, and the tourist class doesn't seem that hard in the very early levels (partly due to the camera flash spell. I think I'll make it do less damage in the next release, it's supposed to be mainly for stunning), but I'm sure its weaknesses get more noticable later on..

**MrCba** · March 25, 2009, 22:50

Originally posted by will_asher

DaJAngband tourists are pretty different from Nethack tourists (although I did give them a handy camera flash spell which stuns monsters for a few turns).

I'll send the savefile to Nick tomorrow, looks like it'll be either an assassin or a tourist.

Ho no, did you mean that I wouldn't have my cat's pet ?

**will_asher** · March 25, 2009, 23:00

Ha! No, pets are definetly beyond my coding ability.

next competition (67)

next competition (67)

Comment

Comment

Comment

Comment

Comment

Comment

Comment

Comment

Comment