12 Dec, 2008, Igabod wrote in the 1st comment:
Votes: 0
I've ignored this for a while now but can't ignore it anymore. In vamp.c and demon.c and a couple other places in the code I get the warning about dam being used uninitialized. I'm not 100% sure what this means but I think it means that something in the function points to dam before it should meaning it needs to be put lower down in the function. This was the case with victim used uninitialized anyway somewhere else. Problem is, when I look at the first function it says that for in vamp.c I can't figure out what part is in the wrong place. Maybe somebody here can help me. Here's the function:

void do_lamprey( CHAR_DATA *ch, char *argument )
{
CHAR_DATA *victim;

char buf[MAX_STRING_LENGTH];

int dam;
int bloodpool;

if (ch->pcdata->stats[UNI_GEN] <= 0)
ch->pcdata->stats[UNI_GEN] = 4;

{
if (IS_SET(ch->newbits, NEW_TIDE))
bloodpool = (3000 / ch->pcdata->stats[UNI_GEN]);
else bloodpool = (2000 / ch->pcdata->stats[UNI_GEN]);
}
if (IS_NPC(ch)) return;
if (!IS_CLASS(ch,CLASS_VAMPIRE))
{
send_to_char("Huh?\n\r",ch);
return;
}
if (ch->pcdata->powers[VAM_OBTE] < 5)
{
send_to_char("You need level 5 Obtenebration to Lamprey.\n\r",ch);
return;

send_to_char("You need level 5 Obtenebration to Lamprey.\n\r",ch);
return;
}


if ( ( victim = ch->fighting ) == NULL )
{
send_to_char( "You aren't fighting anyone.\n\r", ch );
return;

}
WAIT_STATE( ch, 5 );

if (!IS_NPC(victim))
{
dam = ch->pcdata->powers[VAM_OBTE] * 20;
}
else if (IS_NPC(victim))
{
dam = ch->pcdata->powers[VAM_OBTE] * 100;
}
/*
if ( !IS_NPC(victim) && IS_CLASS(victim, CLASS_WEREWOLF) )
{
if (victim->power[DISC_WERE_BOAR] > 2 ) dam *= 0.5;
}
*/
if (is_safe(ch,victim) == TRUE) return;
dam += number_range(1,30);
if ( dam <= 0 )
dam = 1;


sprintf(buf,"Your tendrils of darkness hits $N incredibly hard! [%d]\n\r",dam);
act(buf,ch,NULL,victim,TO_CHAR);
sprintf(buf,"$n's tendrils of darkness hits you incredibly hard! [%d]\n\r",dam);
act(buf,ch,NULL,victim,TO_VICT);
sprintf(buf,"$n's tendrils of darkness hits $N incredibly hard! [%d]\n\r",dam);
act(buf,ch,NULL,victim,TO_NOTVICT);



send_to_char("\n\r",ch);
victim->hit -= dam;
ch->pcdata->condition[COND_THIRST] += number_range(40,50);
if (ch->pcdata->condition[COND_THIRST]>bloodpool)
{
send_to_char("Your bloodlust is sated.\n\r",ch);
ch->pcdata->condition[COND_THIRST] = bloodpool;
}
return;
}


I think the biggest problem with this function is how sloppy it is but maybe thats just me. Anyway, if somebody can see whats in the wrong place please let me know.
12 Dec, 2008, Sharmair wrote in the 2nd comment:
Votes: 0
As was talked about in other threads, when a local (auto) variable is defined, its value is just
what happens to be in that memory when the function is entered. You get the warning you are
getting when the compiler does not see where you initialize the data before using it. In your
case (I am hoping the bad indent in your code is from copy/pasting and not in the real code)
lines 42 to 49 somewhat reformatted:
if (!IS_NPC(victim)){
dam = ch->pcdata->powers[VAM_OBTE] * 20;
}else if (IS_NPC(victim)){
dam = ch->pcdata->powers[VAM_OBTE] * 100;
}

The compiler sees the two ifs but still thinks there could be the case where both are false and
dam remains uninitialized.
The 2nd if could be removed as it is really exactly the same as the else of the first if, like so:
if (!IS_NPC(victim)){
dam = ch->pcdata->powers[VAM_OBTE] * 20;
}else{
dam = ch->pcdata->powers[VAM_OBTE] * 100;
}

The compiler probably will be smart enough to figure dam will always be set. But you could
also (you did not think I would leave you with just one option did you?) do something like:
dam = ch->pcdata->powers[VAM_OBTE] * (IS_NPC(victim))? 100: 20;

And do it in one line.
One way to always make sure a variable in initialized is to set it at the point of definition,
like line 7 in your code do something like:
int dam = 0;
12 Dec, 2008, Igabod wrote in the 3rd comment:
Votes: 0
unfortunately most of the code in low4 is badly indented like that. I've been working almost non-stop trying to solve this but some of it is so screwed up I'm slightly daunted by the task. This particular piece of the code is one of the last few files (alphabetically) so I haven't gotten to it yet. As to your solutions, I did the int dam = 0; and just removed the 2nd if like you said. I tend to find your 2nd option a little confusing. I'm able to figure it out but it takes me longer to understand so I'll stick with the easier format since I tend to drink a little when I code sometimes. These solutions fixed the problem and now I can get to fixing the rest of them. Thank you very much.
12 Dec, 2008, Kline wrote in the 4th comment:
Votes: 0
Just get into the habit of initializing all vars when you declare them and you won't run into this :) It's one of the things I tend to do when going through cleaning up code from whatever base I am using, too.
12 Dec, 2008, Igabod wrote in the 5th comment:
Votes: 0
I have never run into this with anything I wrote, just with the stuff that already existed in low4. The only uninitialized variable I've written in was victim but I fixed that by moving it down in the function to where it was really supposed to be.
12 Dec, 2008, Tyche wrote in the 6th comment:
Votes: 0
Igabod said:
unfortunately most of the code in low4 is badly indented like that. I've been working almost non-stop trying to solve this but some of it is so screwed up I'm slightly daunted by the task. This particular piece of the code is one of the last few files (alphabetically) so I haven't gotten to it yet.


Save yourself from carpal tunnel syndrome and install the 'indent' package.
12 Dec, 2008, Skol wrote in the 7th comment:
Votes: 0
dam = ch->pcdata->powers[VAM_OBTE] * (IS_NPC(victim))? 100: 20;

That works well. Just make sure to keep the if (IS_NPC(victim)) return; part, or it'll try to point to a non-existent part of the structure.
Igabod, what that's doing is an if/else statement in the assignment of 'dam'. It's saying that it's ch->pcdata->powers[VAM_OBTE] *… (ok now check for NPC/PC, if it's NPC then (?) times 100 else (:) times 20.).
So like if you wanted to assign say cup size based upon sex, you'd say something like:
cupsize = BASE_CUP * (IS_FEMALE(victim) ? 2: 5);
int = (some set base size) times (if it's a female (defined previously)(the victim) then 2, else times 5).

It's a very handy way to write sometimes, although sometimes less human readable. Use comments.
12 Dec, 2008, Igabod wrote in the 8th comment:
Votes: 0
yeah I understand what it's doing but it makes it more difficult to determine which number goes to what when you're drunk. I know, I know, you're not supposed to code while drunk but screw that, I'm more creative when I'm drunk.

[edit to add] just now saw your remark about use comments, I would but I like to be able to understand the code without having to rely on comments.
12 Dec, 2008, Sharmair wrote in the 9th comment:
Votes: 0
Quote
Just make sure to keep the if (IS_NPC(victim)) return; part, or it'll try to point to a non-existent part of the structure.

I assume you mean line 18 (note ch not victim) in the original:
if (IS_NPC(ch)) return;

This makes sure the attacker is a player. The code will have problems (and most likely crash) if you use
any player only data. One big thing here is use of data in ch->pcdata. That line should be moved from
line 18 to 9 though, as the code is referencing into ch->pcdata starting at line 10.
A couple other issues I see:
Lines 28 to 30 could just be deleted, as they are both unreachable and redundant.
Line 72 where the victim hp are updated probably should use damage() or the like so victim position is
updated. Even if you don't want some of the things damage() does (and want to keep the direct update
of victim->hit) you probably should do some checks to see if the attack kills the victim and handle
the death.
12 Dec, 2008, Igabod wrote in the 10th comment:
Votes: 0
I used the following:

Quote
if (!IS_NPC(victim)){
dam = ch->pcdata->powers[VAM_OBTE] * 20;
}else{
dam = ch->pcdata->powers[VAM_OBTE] * 100;
}


and the int dam = 0;
12 Dec, 2008, Igabod wrote in the 11th comment:
Votes: 0
Tyche said:
Igabod said:
unfortunately most of the code in low4 is badly indented like that. I've been working almost non-stop trying to solve this but some of it is so screwed up I'm slightly daunted by the task. This particular piece of the code is one of the last few files (alphabetically) so I haven't gotten to it yet.


Save yourself from carpal tunnel syndrome and install the 'indent' package.

I couldn't find the indent package in cygwin, is it not available on cygwin or did I just not look hard enough?

[edit to add] I just used indent in my shell and didn't see any noticeable difference. It's still a messy sloppy code.
12 Dec, 2008, KaVir wrote in the 12th comment:
Votes: 0
Igabod said:
As to your solutions, I did the int dam = 0;


The problem with that approach is that if you later modify the code in such a way that "dam" isn't always properly initialised, the compiler will no longer warn you about it. You'll find people inflicting no damage in certain situations, and you'll have to manually go through the code trying to find out where it's not being properly initialised.
13 Dec, 2008, Igabod wrote in the 13th comment:
Votes: 0
KaVir said:
Igabod said:
As to your solutions, I did the int dam = 0;


The problem with that approach is that if you later modify the code in such a way that "dam" isn't always properly initialised, the compiler will no longer warn you about it. You'll find people inflicting no damage in certain situations, and you'll have to manually go through the code trying to find out where it's not being properly initialised.


So what would you suggest for an appropriate fix for this then Kavir?
13 Dec, 2008, David Haley wrote in the 14th comment:
Votes: 0
Make sure all paths in the control flow set the value to something appropriate. In other words, set the value under all conditions, but only after testing the conditions.
13 Dec, 2008, Igabod wrote in the 15th comment:
Votes: 0
Ok, can I get that again in plain english? I still don't understand a large percentage of coder lingo even after over 8 years of attempting to be a coder.
13 Dec, 2008, Davion wrote in the 16th comment:
Votes: 0
What he's saying is to make sure that on the path of the function, make sure the variable is set. Do not look at conditional initialization for variables (if you assume it'll always hit a condition) instead assume that it'll fail every condition (that's what the compiler thinks.) Somewhere in there you should set it to a value regardless of the conditions set. Either by setting the value in the direct scope of the function or in something else (like an else clause!) of the function.
13 Dec, 2008, David Haley wrote in the 17th comment:
Votes: 0
Control flow means the paths that your program can take. For example,

i = 1;
if (condition)
j = 1;
else
j = 2;
k = 3;

In this very simple program, i will be 1, k will be 3, and the control flow path can split at the if statement.

Notice that there are two possible outcomes:
i = 1, j = 1, k = 3
and
i = 1, j = 2, k = 3

You can draw a graph of your program's execution by making a branch at every if statement and then rejoining branches when the if statements finish. Things get a little more complicated with loops but the idea is the same.

Now consider this program:
int i = 1;
int j;
if (condition)
j = 2

There are, again, two possible paths in the control flow graph:
i = 1, j = 2
and
i = 1, j = ???

The second path is bad if you are going to use j somehow. If you don't set j but never use it, it's not really a problem.



So, to solve your issue, you need to make sure that all control flow paths set a value for your variable before every point at which the variable is used. That means following all the if statement branching and making sure that each one assigns a reasonable value to the variable.
13 Dec, 2008, Sharmair wrote in the 18th comment:
Votes: 0
The part of the fix you did with removing the 2nd if (and just using the else) should cover
all control paths and fix the warning alone. The setting at the point of definition should (ideally)
only be needed if your logic is too complex for the compiler to realize the variable is in fact not
ever used uninitialized or the value you set really has meaning if it is never changed later.
13 Dec, 2008, David Haley wrote in the 19th comment:
Votes: 0
It's ok to set a value in the initializer if it is truly a meaningful default value, and you know that you will set it to something else later on. I agree with the others who have said that initialization should be avoided if your intent is to define it according to various conditions, because it can hide uninitialized-variable warnings from the compiler or debugging tools like valgrind.
13 Dec, 2008, Igabod wrote in the 20th comment:
Votes: 0
Ah ok, I believe I'm understanding this a bit more clearly. Although Davion's attempt at making it easier to understand was almost confusing, I had to read it a couple times before I grasped what he was saying due to the use of the i, j, and k but I appreciate the help nevertheless (is that one word or hyphenated?).
0.0/29