Where's the mistake???

[Gordon Burditt]

compiler says: function undeclared how come???

Hint: spelling counts.
Hint: There is no standard C function called malllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllloc().
Hint: don't cast the return value of malloc().

Gordon L. Burditt

 

[Christopher Benson-Manica]

Don't you enable suitable warnings? This class of mistake should
be pointed out by the compiler. But even if not, then the linker
message should make the error obvious.

I've had the misfortune of making this error in such a way that the
compiler was not obliged to complain (it was an "errror" in a strstr
test where I had intended the correctly-spelled version).

 

[Barry Schwarz]

I need to write a program that generates 5 random numbers and puts them into
a linked list. (Print the list) From that list it forms another list in a
way if the arrangement of elements in the first one was 5 8 3 1 in the new
one it should be
1 3 8 5 (print the new list)

where's the error now ??

Which error do you mean? You have a syntax error. You have many
logic errors. You don't check to see if malloc succeeded. You create
memory leaks. You have global variables that don't need to be. You
cast the return from malloc. You use non-standard features.

The answer to your question is: there is more than one and they are
everywhere.

#include<stdio.h>
#include<stdlib.h>
#include<time.h>

struct lista{
int element;
struct lista *next;
}*pocetak;

main()
{
struct lista *q, *nova;
int i;
pocetak=NULL;
srand(time(NULL));
for(i=0;i<5;i++)
{
q=(struct lista*) malloc(sizeof(struct lista));

You should check that malloc succeeded.

q->element=rand()%100;
pocetak=q;
q->next=NULL;
}

While this loop does allocate 5 different blocks of memory, you throw
the addresses of the first four away. This creates memory leaks.

q=pocetak;
printf("Nasumicni brojevi:\n");
while(q!=NULL)

This loop is guaranteed to execute at most one time.

{
printf("\n%d",q->element);
q=q->next;
}

q=pocetak;
for(i=0;i<5;i++)
{
nova=(struct lista*) malloc(sizeof(struct lista));
nova->element=pocetak+(5-i)*(sizeof(struct lista));

This is a syntax error. element is an int. pocetak is a pointer.
The expression involves pointer arithmetic which will evaluate to a
pointer. You cannot assign a pointer to an int this way.

pocetak=nova;
nova->next=NULL;
}

This loop has the exact same memory leak problem.

nova=pocetak;
printf("\nNaopako:\n");
while(nova!=NULL)

This loop also executes at most once.

{
printf("\n%d",nova->element);
nova=nova->next;
}
system("pause");

This is not particularly portable. Many use a getchar to wait for a
key press.

return 0;
}

<<Remove the del for email>>

 

[Jack Klein]

You'll want to have a second look at this line here.

(Besides, you don't need the cast.)

Actually he might need the cast, depending on what mallloc() returns.

 

[kal]

ok I rewrote the code, no mistakes but it still doesn't work ...

Let us consider the first part. The objective is to allocate
a linked list of 5 structure instances. Schematically the first
two are as follows where "v" is the value you store (element)
and "p" is the pointer to the next structure (next.)
_______ _______
| v | p | .-> | v | p | .->
------- | ------- |
|____| |____|

We deduce that (1) we should store the first or starting structure
address somewhere where we can get at it, this is the "pocetak"
pointer that is allocated golbally, and (2) we should have access
to the currently allocated structure and the PREVIOUS structure,
if any, so that we can store the current structure address in the
"next" item of the previous structure.

Let us now do the coding.

#include<stdio.h>
#include<stdlib.h>
#include<time.h>
ok.

struct lista{
int element;
struct lista *next;
}*pocetak;

"pocetak" stores the address of the very first structure.

main()
{
struct lista *q, *nova;

"q" will have the previous structure address, if any.
"nova" has the currently allocated structure address.

int i;
pocetak=NULL;
srand(time(NULL));

"pocetak" is initialized with NULL to start with.

for(i=0;i<5;i++)
{
q=(struct lista*) malloc(sizeof(struct lista));
q->element=rand()%100;
pocetak=q;
q->next=NULL;
}

Here the LAST allocated structure address is stored in "pocetak."
But what is required is the FIRST allocated structure address.
Also, nothing is stored in the "next" item.

What is needed is something like the following.

for(i=0;i<5;i++)
{
nova=(struct lista*) malloc(sizeof(struct lista));
nova->element=rand()%100;
nova->next=NULL;
if (pocetak == NULL) /* or i==0 */
pocetak=nova;
else
q->next=nova; /* i>0 hence q will be valid */
q=nova; /* store the last address in q */
}

q=pocetak;
printf("Nasumicni brojevi:\n");
while(q!=NULL)
{
printf("\n%d",q->element);
q=q->next;
}

Ok. Include the following for salutations after output.

printf("\n\nSayonara\n\n");

system("pause");
return 0;
}

Ok.

 

[Peter Shaggy Haywood]

Groovy hepcat Nikola was jivin' on Mon, 14 Jun 2004 19:11:24 +0200 in
comp.lang.c.
Re: Where's the mistake???'s a cool scene! Dig it!

ok I rewrote the code, no mistakes but it still doesn't work ...

Well, that was a precise and to the point description of the
problem! You wanna narrow it down, just a tad? What do you mean by "it
still doesn't work"? Does it run and display incorrect output? Does it
run and display no output? Does it not run at all? Does it crash at a
certain point? Does it run indefinitely? Does it not compile? And if
not, what are errors reported by the compiler? Help us to help you by
being specific.

#include<stdio.h>
#include<stdlib.h>
#include<time.h>

struct lista{
int element;
struct lista *next;
}*pocetak1,*pocetak2;

Global variables are bad, unless you have a good reason to use them.
But in a program that occupies only one function, they are utterly
pointless. Put these variable definitions in main().

main()

This will be rejected by any C99 conforming compiler. Implicit int
return type was removed from the standard nearly five years ago. You
must, now, specify the return type. (In main()'s case this is int, of
course. I see by the return statement you know that already, but it
bears spelling out for any newbies who may be reading this.) It was
always a good idea to specify the return type of all functions anyhow.
It is also, and has always been, a good idea to supply a prototype for
every function. As I hope you are aware, a prototype is a function
declaration that specifies the number and types of the parameters.
Even a function that takes no arguments can benefit from a prototype.
So, provide a prototype and specify a return type like so (remembering
that a function definition is also a declaration and can, therefore,
provide a prototype):

int main(void)

{
struct lista *q, *nova;
int i;
pocetak1=NULL;

This line, though not an error, is pointless, since pocetak1 is
defined at file scope, and, therefore, is already implicitly
initialised to a null pointer value. (I notice you didn't do the same
for pocetak2 though.)

srand(time(NULL));
for(i=0;i<5;i++)
{
q=(struct lista*) malloc(sizeof(struct lista));

Don't cast malloc()'s return. You have been told this before. Do
CHECK malloc()'s return, in case of an allocation error. Also, use the
thing being allocated, not the type, as the operand of sizeof. Ie.,
use sizeof *q, not sizeof(struct lista). The reasons for this have
been expounded time and time again here, so I won't repeat them.
It is your resposibility to find out these things before you post.
You should have read at least a month worth of newsgroup posts first.
And you should have read the FAQ
(http://www.eskimo.com/~scs/C-faq/top.html) second.

q->element=rand()%100;

This isn't the best way to get a random number within a certain
range. But it'll do for now, and I'll leave it to you to consult the
FAQ on that.

q->next=pocetak1;
pocetak1=q;
}
q=pocetak1;
printf("Random numbers:\n");
while(q!=NULL)
{
printf("\n%d",q->element);
q=q->next;
}
printf("\nOther way round:\n");
q=pocetak1;
for(i=0;i<5;i++)
{
nova=(struct lista*) malloc(sizeof(struct lista));

See above.

nova->element=(q+(5-i)*sizeof(struct lista))->element;

Huh? What is that supposed to achieve? You just entered undefined
behaviour territory. A linked list is not an array. You need to follow
the links to traverse the list.

q=q->next;
nova=nova->next;

nova->next has not been given a value before this, so you again
invoke undefined behaviour.

nova->next=pocetak2;
pocetak2=nova;
}
nova=pocetak2;
while(nova!=NULL)
{
printf("\n%d",nova->element);
nova=nova->next;
}
system("pause");

What is this system specific dreck supposed to be in aid of?

return 0;
}

I have tried to point out all the problems and potential problems in
your code, but I may have missed something. The rest is up to you.

--

Dig the even newer still, yet more improved, sig!

http://alphalink.com.au/~phaywood/
"Ain't I'm a dog?" - Ronny Self, Ain't I'm a Dog, written by G. Sherry & W. Walker.
I know it's not "technically correct" English; but since when was rock & roll "technically correct"?