How to fix this structure?

[Chris R.]

Hi everyone. I am trying to finish my homework, but I seem not to
figure out how to fix this structure that seems to make wrong output.
What this program should do is use structure to save the word and line
number on which it was in array pointer using malloc and print them
out alphabetized (no duplicates).

Most of it works, except the words seem to be corrupted. Here is a
code, if anyone can help me it would help so much since I've been
working on this for a really long time. I tried even changing '\0' to
NULL, but then program errored out.


Here is my code:



#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>


typedef struct list
{
int linenum;
char word[30];
} LIST;



void main()
{
LIST *p[1000], *holder;
int line = 1;
int pcounter = 0;
char buffer[40];
int buffercounter = 0;
char letter = ' ';
int inword = 0;
int i;
int compare;
int shouldstore = 1;
int x,y;


while (letter != EOF)
{
letter = getchar();
if ( (letter >= 'a' && letter <= 'z') || (letter >=
'A' && letter <= 'Z') )
{
letter = toupper(letter);
inword = 1;
buffer[buffercounter] = letter;
buffercounter++;
}
else if (letter == '\n')
{
line++;
if (inword == 1)
{
inword = 0;
buffercounter++;
buffer[buffercounter] = '\0';
buffercounter = 0;
if (pcounter == 0)
{
p[pcounter] = (LIST*) malloc (sizeof(LIST));
strcpy( p[pcounter]->word , buffer);
p[pcounter]->linenum = line;
pcounter++;
}
else
{
for (i = 0; i < pcounter; i++)
{
compare = strcmp(p->word, buffer);
if (compare == 0)
{
shouldstore = 0;
}
}
if (shouldstore == 0)
{
shouldstore = 1;
}
else
{
p[pcounter] = (LIST*) malloc (sizeof(LIST));
strcpy( p[pcounter]->word, buffer);
p[pcounter]->linenum = line;
pcounter++;
}
}

}
else
{
buffercounter = 0;
}
}
else
{
if (inword == 1)
{
inword = 0;
buffercounter++;
buffer[buffercounter] = '\0';
if (pcounter == 0)
{
p[pcounter] = (LIST*) malloc (sizeof(LIST));
strcpy( p[pcounter]->word , buffer);
p[pcounter]->linenum = line;
pcounter++;
}
else
{
for (i = 0; i < pcounter; i++)
{
compare = strcmp(p->word, buffer);
if (compare == 0)
{
shouldstore = 0;
}
}
if (shouldstore == 0)
{
shouldstore = 1;
}
else
{
p[pcounter] = (LIST*) malloc (sizeof(LIST));
strcpy( p[pcounter]->word, buffer);
p[pcounter]->linenum = line;
pcounter++;
}
}
}
else
{
buffercounter = 0;
}
}


}

for(x = 0; x < pcounter; x++)
for(y = 0; y < pcounter-1; y++)
{
compare = strcmp(p[y]->word, p[y+1]->word);
if( compare < 0)
{
holder = p[y+1];
p[y+1] = p[y];
p[y] = holder;
}
}

for (x = 0; x < pcounter; x++)
{
printf("\nThe word %s was on line %d", p[x]->word, p[x]->linenum);
}
}

 

[Kevin D. Quitt]

void main()

Your first problem: this makes your program invoke undefined behaviour.
Instead, use:

int main(void)

else if (letter == '\n')
{
line++;
if (inword == 1)
{
inword = 0;
buffercounter++;

Remove this last line. buffercounter already points one past the end of
the string, so this allows an extra character to creep in.

buffer[buffercounter] = '\0';

Instead of this here, you could put

memset( buffer, 0, sizeof buffer );

at the end of else clause, and eliminate both of the previous lines.

Now, shall we talk about checking to make sure that entered words aren't
too long? That buffer can hold 40 characters, but the structure can only
hold 30? Instead of strcpy, use strncpy, otherwise you're writing
Micorosft-quality code.

I didn't look any farther than this, since your only complaint was the
corruption of the words on the list.

 

[CBFalconer]

Hi everyone. I am trying to finish my homework, but I seem not to
figure out how to fix this structure that seems to make wrong output.
What this program should do is use structure to save the word and line
number on which it was in array pointer using malloc and print them
out alphabetized (no duplicates).

Most of it works, except the words seem to be corrupted. Here is a
code, if anyone can help me it would help so much since I've been
working on this for a really long time. I tried even changing '\0' to
NULL, but then program errored out.

Here is my code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>

typedef struct list
{
int linenum;
char word[30];
} LIST;

void main()
{
LIST *p[1000], *holder;
int line = 1;
int pcounter = 0;
char buffer[40];
int buffercounter = 0;
char letter = ' ';
int inword = 0;
int i;
int compare;
int shouldstore = 1;
int x,y;

while (letter != EOF)

I stopped here, because you already have two glaring errors. main
returns int, not void, so say and do so. Second, a char cannot
hold EOF, so letter must be an int.

Fix your excessive indentation. Use spaces, not tabs. Don't cast
malloc. Limit line length to 65 chars for usenet posting.

 

[Default User]

buffer[buffercounter] = '\0';

Instead of this here, you could put

memset( buffer, 0, sizeof buffer );

at the end of else clause, and eliminate both of the previous lines.

Why do you think that is an improvement?




Brian Rodenborn

 

[Kevin D. Quitt]

Why do you think that is an improvement?

Stewardship. When you have a buffer full of nuls, you always have a valid
string that contains no detritus. If this were in a time-critical section
of code, I wouldn't use memset, but for clean operation I would prefer it.

 

[Default User]

Stewardship. When you have a buffer full of nuls, you always have a valid
string that contains no detritus. If this were in a time-critical section
of code, I wouldn't use memset, but for clean operation I would prefer it.

Who cares what the rest of the memory buffer has in it when you are
dealing with strings?

What happens when the section of code is cut out and put in a function
which has the buffer passed in?

What happens if the user gets away from using those fixed-sized char
buffers and moves to dynamically allocated ones?


If your goal is to null-terminate a string, then null-terminate the
string. White-washing an entire buffer makes no sense and was poor
advice to give to a newbie.



Brian Rodenborn

 

[Mabden]

it.

Who cares what the rest of the memory buffer has in it when you are
dealing with strings?

What happens when the section of code is cut out and put in a function
which has the buffer passed in?

What happens if the user gets away from using those fixed-sized char
buffers and moves to dynamically allocated ones?


If your goal is to null-terminate a string, then null-terminate the
string. White-washing an entire buffer makes no sense and was poor
advice to give to a newbie.

ummmm... I use calloc() sometimes...

....am i bad?

 

[Kevin D. Quitt]

Who cares what the rest of the memory buffer has in it when you are
dealing with strings?

It's common for newbies to mess up string termination, especially when
they build strings by themselves. The method I suggested prevents
termination problems and makes debugging easier because the strings are
clearly delineated in memory.

What happens when the section of code is cut out and put in a function
which has the buffer passed in?
What happens if the user gets away from using those fixed-sized char
buffers and moves to dynamically allocated ones?

These two are the same question. And the answer is easy: you can zero the
buffer by using either the length passed to the function, or to some
guaranteed-minimum size if you're dumb enough not to pass the size. And
what happens when they forget to nul-terminate the character array or put
the nul in the wrong place?

If your goal is to null-terminate a string, then null-terminate the
string.

My goal is to write code that is easy to debug and that works because it's
supposed to, and not by accident.

White-washing an entire buffer makes no sense and was poor
advice to give to a newbie.

Say rather that you can't see the sense in it; that's one of your
problems, not mine.

 

[Default User]

It's common for newbies to mess up string termination, especially when
they build strings by themselves. The method I suggested prevents
termination problems and makes debugging easier because the strings are
clearly delineated in memory.

So you advocate some cargo-cult programming to prevent them from making
some mistake, instead of teaching them what null-termination is, what it
does, and how to successfully do it at the correct point in the program?

These two are the same question. And the answer is easy: you can zero the
buffer by using either the length passed to the function, or to some
guaranteed-minimum size if you're dumb enough not to pass the size.

Adding needless complexity to the program for no benefit.

And
what happens when they forget to nul-terminate the character array or put
the nul in the wrong place?

Then they have programming error. One that should be easy to diagnose
and locate, once they've been taught how strings work, rather than
covering up the situation by blanking out entire buffers.

My goal is to write code that is easy to debug and that works because it's
supposed to, and not by accident.

Yet your proposed solution does none of that.

Say rather that you can't see the sense in it; that's one of your
problems, not mine.

As you say. However that same answer can be given to any disagreement
about programming methodology. I prefer to attack the code, not the
coder in this case.

I'm basing my assertions on my experiences. You may have different ones.



Brian Rodenborn

 

[Mark McIntyre]

Who cares what the rest of the memory buffer has in it when you are
dealing with strings?

the maintenance team do, when you go and strncpy something into it.

 

[Dan Pop]

It's common for newbies to mess up string termination, especially when
they build strings by themselves. The method I suggested prevents
termination problems and makes debugging easier because the strings are
clearly delineated in memory.

The method you suggested merely masks the newbie's mistake of forgetting
the string terminator. It's good for people who know what they're doing
so that they can deliberately omit the string terminator, but I wouldn't
include newbies in this category.

Dan

 

[CBFalconer]

It's common for newbies to mess up string termination, especially
when they build strings by themselves. The method I suggested
prevents termination problems and makes debugging easier because
the strings are clearly delineated in memory.

When you use binary comparison of output files to verify that
changes have not harmed program action, you get highly upset at
programmers who leave areas of that output holding random junk.
If it must be junk, at least let it be junk generated by that
particular program run. However, the preferred junk value is all
bits zero.

 

[Kevin D. Quitt]

I'm basing my assertions on my experiences. You may have different ones.

Clearly. My specialty is ultra-reliable embedded systems; I have software
on several operational spacecraft, numerous medical devices, and on
large-scale industrial metal-forming robots. Bugs are not an option.

 

[Richard Bos]

the maintenance team do, when you go and strncpy something into it.

Then don't use strncpy(). It's a broken function, anyway. Use strncat().

Furrfu...

Richard

 

[Dan Pop]

Then don't use strncpy(). It's a broken function, anyway. Use strncat().

Nonsense! strncpy() is a perfectly good function *if* used for the
purpose it was designed: manipulating a data format not quite unlike C
strings. Its only objectionable feature is its name, which starts with
the str prefix, misleading the people who are too lazy to properly read
the specifications of the functions they use.

Dan

 

[Mark McIntyre]

the maintenance team do, when you go and strncpy something into it.

Then don't use strncpy(). It's a broken function, anyway. [/QUOTE]

This may be true, but its largely irrelevant don't you think?

Use strncat().

Since this function does something different. I'm not absolutely convinced
you can use it.

 

[Richard Bos]

Then don't use strncpy(). It's a broken function, anyway.

This may be true, but its largely irrelevant don't you think? [/QUOTE]

Since strncpy() doesn't occur in the OP's code, it itself is irrelevant
to this thread, yes. Its brokenness is not really irrelevant to
maintenance - I'd certainly distrust code that memset() an array and
then strncpy()d into it, if only because strncpy() _itself_ already
zero-pads its destination.

Bork, bork, bork.

Richard

 

[J. J. Farrell]

Since strncpy() doesn't occur in the OP's code, it itself is irrelevant
to this thread, yes. Its brokenness is not really irrelevant to
maintenance - I'd certainly distrust code that memset() an array and
then strncpy()d into it, if only because strncpy() _itself_ already
zero-pads its destination.

Er - but only sometimes. Isn't this why it's often regarded as
broken? Or am I missing the point here?

 

[Richard Bos]

Er - but only sometimes. Isn't this why it's often regarded as
broken? Or am I missing the point here?

Yes, it is. But consider:

char buf[SIZE];
memset(buf, 0, SIZE);
strncpy(buf, src, SIZE);

This is incorrect. strncpy() copies exactly SIZE chars into buf,
overwriting _all_ of the memory you've set. Worse, if scr is at least
SIZE chars long, excluding its terminal null, you've just unterminated
your buffer, and what you have is no longer a string.
All very well, you say, but that code is broken. Just correct it to:

char buf[SIZE+1];
memset(buf, 0, SIZE+1);
strncpy(buf, src, SIZE);

Sure, that's correct. The terminating null is now guaranteed. It's also
quite inefficient, since you're overwriting the first SIZE chars of buf
twice - once using memset(), once using strncpy(). All you need to write
is the last char of buf; you don't need to blot the entire array. This
is sufficient:

char but[SIZE+1];
buf[SIZE]=0;
strncpy(buf, src, SIZE);

And that's why I say that code which uses memset() on an area it then
strncpy()'s into is dubious: at the very least, it suggests that the
programmer wasn't very clear on what strncpy() actually _does_.

Richard