Problems with my little c-program

[Henk]

Hi Guys,

(see actual code below)

I wrote a little program that is supposed to read a file (input.txt)
into memory (using a stack) and then reverse the file and display it to
output. It works, but I still get some errors when i try to compile
it:

stack.c: In function 'reverseFile'
stack.c:98: warning:int format, pointer arg (arg 2)

And when I run the program It does reverse the input.txt file and
displays it to output, but I am getting 2 more errors:

free() invalid pointer 0x804a178!
and in the end i get a:
Segmentation fault.
I know it is not the best C code ever, I am just getting started.I did
some printf's in the code just for testing where it gets.

I hope you can help me.

Greetings Henk

The code--------------------

-----------stack.c-------------------
#include <stdio.h>
#include <stdlib.h>
#include "stack.h"
struct buffer* readFileToBuffer(char *filename){
struct buffer *first;
struct buffer *current;

FILE *file =fopen(filename,"r");
if(file==NULL){
printf("Unable to open: %s\n", filename);
exit(1);
}

first = createBuffer();
current = first;

while( !(feof(file))){ //VERANDERD
//leest hele file in, maakt buffers aan en zet de file daarin
current->size=
fread(current->data,(sizeof(char)),BUFFER_SIZE,file);

if(current->size < BUFFER_SIZE){
if(ferror(file) != 0){
printf("file error, readFileToBuffer()\n");
clearerr(file);
exit(1);
}
if(feof(file) !=0){
printf("end of file\n");
}
}
if(feof(file)==0){
//alleen maken als feof nog niet gezet is. Anders overbodige
buffer.
struct buffer *nextbuf=createBuffer();
current->next = nextbuf;
current=current->next;
}
}
clearerr(file);
fclose(file);
return first;
}

struct buffer* createBuffer(){
struct buffer *buf;

buf=malloc(sizeof(struct buffer));
if (buf==NULL){
printf("Not possible to allocate memory\n");
exit(1);
}
buf->size = 0;
buf->next = NULL;
buf->data = malloc(sizeof(char)*BUFFER_SIZE);

if (buf->data == NULL){
printf("Not possible to allocate memory\n");
exit(1);
}
return buf;
}
void printNormalFile (struct buffer *firstElem){
struct buffer *current = firstElem;

while((*current).next != NULL){
printf((*current).data);
printf("current->lenght=%d\n",current->size);
current = (*current).next;
}
printf((*current).data);
printf("current->lenght=%d\n",current->size);
}
void reverseFile(struct buffer *first){
printf("frst->lenght=%d\n",first->size);
struct buffer *current = first;
struct buffer *previousElm = NULL;
int laatsteElm;
int i;
char *kopie;

while(1){
if(current==NULL){
break;
}
while(current->next != NULL){ //laatste buffer zoeken
previousElm=current;
current=current->next;
printf("\na\n");
}
printf("\ncurrent->lng=%d\n",current->size);
printf("prev=%d\n",previousElm);
laatsteElm = current->size;
kopie=(current->data)+(current->size)-1;
for(i=0;i < laatsteElm;i++){
printf("%c",*kopie);
kopie--;
}
free(current->data);
free(current);
current=first;
if(previousElm==NULL){
return;
}
previousElm->next=NULL;

}
}

int main(int argc, char** argv){

char *filename=argv[1];

struct buffer *first=readFileToBuffer(filename);
printf("firstsize=%d",first->size);
printNormalFile(first);
reverseFile(first);
return 0;
}

-------------------------------------------------------------------

---------stack.h---------------------------

#ifndef STACK
#define STACK

#define BUFFER_SIZE 128
#include <stdlib.h>

struct buffer{
char *data;
struct buffer *next;
int size;
};

struct buffer* readFileToBuffer(char *filename);
struct buffer* createBuffer();
void printNormalFile (struct buffer *firstElem);
void reverseFile(struct buffer *first);

#endif
-------------------------------------------------

[Richard Bos]

"Henk" <cc*****@hotmail.com> wrote:

stack.c: In function 'reverseFile'
stack.c:98: warning:int format, pointer arg (arg 2)

And when I run the program It does reverse the input.txt file and
displays it to output, but I am getting 2 more errors:

free() invalid pointer 0x804a178!
and in the end i get a:
Segmentation fault. void reverseFile(struct buffer *first){
printf("frst->lenght=%d\n",first->size);
struct buffer *current = first;
struct buffer *previousElm = NULL;
int laatsteElm;
int i;
char *kopie;

while(1){
if(current==NULL){
break;
}
while(current->next != NULL){ //laatste buffer zoeken
previousElm=current;
current=current->next;
printf("\na\n");
}
Note that this loop does not set previousElm if you have a one-node list
(i.e., when current->next starts out null). Since this function breaks
down the list, you will eventually encounter this situation. At that
point, either previousElm will contain garbage (if first->next, as
passed to the function, is null); or it will contain the value from the
previous loop, which is then the same as current.
printf("\ncurrent->lng=%d\n",current->size);
printf("prev=%d\n",previousElm);
This line causes the warning, since previousElm is a pointer (to a
struct buffer, to be precise), and %d expects an int. To print a
pointer, do this:

printf("prev=%p\n", (void *)previousElm);

(Since printf() is a variadic function, the cast to void * is important,
btw.)
laatsteElm = current->size;
kopie=(current->data)+(current->size)-1;
for(i=0;i < laatsteElm;i++){
printf("%c",*kopie);
kopie--;
}
free(current->data);
free(current);
current=first;
if(previousElm==NULL){
return;
}
previousElm->next=NULL;

I don't know why free() complains, but when previousElm == current, this
line scribbles over memory that has already been free()d.

Richard

[Henk]

Thanks for the info. I solved the "printf" problem thanks to your info.
But I am still nog capable to solve the second problem:

Note that this loop does not set previousElm if you have a one-node list
(i.e., when current->next starts out null). Since this function breaks
down the list, you will eventually encounter this situation. At that
point, either previousElm will contain garbage (if first->next, as
passed to the function, is null); or it will contain the value from the
previous loop, which is then the same as current.
and the following as mentioned by Richerd as wel:
when previousElm == current, this
line scribbles over memory that has already been free()d

Could someone help me out, I 'm a Rooky in C programming (just started
3 weeks ago). And was glad that I could make this more ore less work.
But I cannot see what I should do to solve my problem.

Greetings Henk (The C Rooky)

[Michael Wojcik]

In article <11**********************@f14g2000cwb.googlegroups .com>, "Henk" <cc*****@hotmail.com> writes:

stack.c: In function 'reverseFile'
stack.c:98: warning:int format, pointer arg (arg 2)
This is the line in question:
printf("prev=%d\n",previousElm);
and "previousElm" is a struct buffer *. %d is the format specifier
for int arguments. A pointer is not an int. There are a number of
ways to print the value of an object pointer; the simplest is to use
the %p format specifier and cast the pointer to a void *:
printf("prev=%p\n", (void *)previousElm); And when I run the program It does reverse the input.txt file and
displays it to output, but I am getting 2 more errors:

free() invalid pointer 0x804a178!
I recommend using a debugger. There are only two calls to free
in the code you posted; it'd be trivial to insert a breakpoint
and see what's happening at each call to free.

(You'll need a list with at least two elements in it to see the
error. Look at what values previousElm has if you make at least
two iterations in the loop in reverseFile.)

Some additional comments:
while( !(feof(file))){ //VERANDERD
Don't do this; test the result of fread. See the comp.lang.c FAQ.
fread(current->data,(sizeof(char)),BUFFER_SIZE,file);
sizeof(char) is always 1. And why the extra parentheses around it?
clearerr(file);
fclose(file);
There's no point in calling clearerr on a FILE* that you're about
to fclose.
struct buffer* createBuffer(){
If a function takes no parameters, it should be declared with a
parameter list of "void":

struct buffer *createBuffer(void){

Also, note that it's widely considered poor style to separate the
"*" and the identifier for an identifier to pointer type. It leads
to errors such as:

int* ptrA, ptrB;

where, contrary to its name, "ptrB" is actually an int, not an int
pointer.
buf=malloc(sizeof(struct buffer));
buf = malloc(sizeof *buf);

is better; if the type of buf changes, the argument to malloc will
still be correct.
buf->data = malloc(sizeof(char)*BUFFER_SIZE);
Again, sizeof(char) is always 1.
while((*current).next != NULL){
(*current).next is the same as current->next. That's the whole
point of the arrow operator.
printf((*current).data);
Bad idea. What if there's a percent-sign character in the data?
This is a classic "format-string bug". Use one of:

puts(current->data); /* this will append a newline */
fputs(current->data, stdout); /* this will not */
printf("%s\n", current->data); /* equivalent to puts */
printf("%s", current->data); /* equivalent to fputs */

Note there's no reason to prefer the printf alternatives. In
general, use printf if you're formatting; to print only a single
string, use puts/fputs.
while(1){
if(current==NULL){
break;
}
while (current != NULL){

has the same semantics and is clearer. However, you have another
loop invariant buried in the loop:
if(previousElm==NULL){
return;
}
and it's almost at the end of the loop (where while's control-
expression would be evaluated), so it'd be even better to
write:

previousElm = current;
while (current != NULL && previousElm != NULL){

or the terser:

previousElm = current;
while (previousElm){

since in this version, the only time current can be null is if
reverseFile is called with a null pointer, and in that case
previousElm will also be null on the first loop iteration.

This assumes you've corrected the bug I alluded to above.
int main(int argc, char** argv){

char *filename=argv[1];

I realize this is only a learning exercise, but it's still a
good idea to check whether you have an argv[1] before trying to
use it. (And filename is extraneous here, though I suppose you
might want to use it for the sake of clarity.)

By the way, you're not using a stack here, in the conventional sense.
You have a linked list, and when you reverse the file you're
processing the list in reverse. The whole point of a stack is that
each new element is added to the *front* of the stack, so that when
you process the stack in order, you get the input in reverse.

If you actually need to implement this with a stack, I'd recommend
starting with the simplest implementation: one character per node,
and use fgetc to read one character at a time. While it has
tremendous overhead it would let you see exactly what's happening.
Then when it's working you can optimize it.

--
Michael Wojcik mi************@microfocus.com

Please enjoy the stereo action fully that will surprise you. -- Pizzicato Five

[Henk]

Thanks for the good explanation, it really helped.
Hope to learn from my mistakes and get better.

Greetings Henk

[Barry Schwarz]

On 4 Oct 2005 05:02:25 -0700, "Henk" <cc*****@hotmail.com> wrote:

Hi Guys,

(see actual code below)

I wrote a little program that is supposed to read a file (input.txt)
into memory (using a stack) and then reverse the file and display it to
output. It works, but I still get some errors when i try to compile
it:

stack.c: In function 'reverseFile'
stack.c:98: warning:int format, pointer arg (arg 2)

And when I run the program It does reverse the input.txt file and
displays it to output, but I am getting 2 more errors:

free() invalid pointer 0x804a178!
and in the end i get a:
Segmentation fault.
I know it is not the best C code ever, I am just getting started.I did
some printf's in the code just for testing where it gets.

I hope you can help me.
I rearranged your functions so you can read the comments in
chronological order.

Greetings Henk

The code--------------------
---------stack.h---------------------------

#ifndef STACK
#define STACK

#define BUFFER_SIZE 128
#include <stdlib.h>

struct buffer{
char *data;
struct buffer *next;
int size;
};

struct buffer* readFileToBuffer(char *filename);
struct buffer* createBuffer();
It is better to specify void to explicitly state there are no
arguments.
void printNormalFile (struct buffer *firstElem);
void reverseFile(struct buffer *first);

#endif
-------------------------------------------------

-----------stack.c-------------------
#include <stdio.h>
#include <stdlib.h>
#include "stack.h"

int main(int argc, char** argv){

char *filename=argv[1];
You need to check that argc is at least 2.

struct buffer *first=readFileToBuffer(filename);
printf("firstsize=%d",first->size);
printNormalFile(first);
reverseFile(first);
return 0;
}
struct buffer* readFileToBuffer(char *filename){
struct buffer *first;
struct buffer *current;

FILE *file =fopen(filename,"r");
if(file==NULL){
printf("Unable to open: %s\n", filename);
exit(1);
}

first = createBuffer();
current = first;
first and current now both point to the same node. The node points to
a 128 byte buffer which is uninitialized.

while( !(feof(file))){ //VERANDERD
//leest hele file in, maakt buffers aan en zet de file daarin
current->size=
fread(current->data,(sizeof(char)),BUFFER_SIZE,file);
This will read 128 bytes into the buffer without regard to the values.
(Since you opened the file in text mode, it is possible that your
system will intercept a certain value as an EOF indicator. Let's
assume your file doesn't have any of these values.)

Unfortunately, there is no reason to assume that any of the 128 values
just read is '\0'. Consequently, the buffer does not contain a
string.

Were you perhaps thinking about using fgets?

if(current->size < BUFFER_SIZE){
if(ferror(file) != 0){
printf("file error, readFileToBuffer()\n");
clearerr(file);
exit(1);
}
if(feof(file) !=0){
printf("end of file\n");
}
}
if(feof(file)==0){
//alleen maken als feof nog niet gezet is. Anders overbodige
buffer.
struct buffer *nextbuf=createBuffer();
current->next = nextbuf;
current=current->next;
current now points to the next node in the list (which in turn points
to a new buffer) and you continue reading. Unless your file is an
exact multiple of 128 bytes, the last buffer will not be completely
filled. If it is an exact multiple, the last buffer will be empty
(actually completely uninitialized).
}
}
clearerr(file);
fclose(file);
return first;
}

struct buffer* createBuffer(){
struct buffer *buf;

buf=malloc(sizeof(struct buffer));
if (buf==NULL){
printf("Not possible to allocate memory\n");
exit(1);
}
buf->size = 0;
buf->next = NULL;
buf->data = malloc(sizeof(char)*BUFFER_SIZE);

if (buf->data == NULL){
printf("Not possible to allocate memory\n");
A different error message would be useful to distinguish between this
error and the previous one.
exit(1);
}
return buf;
}
void printNormalFile (struct buffer *firstElem){
struct buffer *current = firstElem;

while((*current).next != NULL){
printf((*current).data);
You are telling printf to use the string in current buffer as a format
string. Two problems:

As noted above, it might not be a string. This would lead to
undefined behavior.

If the data in your file contains any '%', printf will treat it as
a format specifier. Since you have no corresponding arguments in the
argument list, this also would invoke undefined behavior. If the data
in the buffer was a string, you would be better off with
printf ("%s", current->data);
or
fputs(current->data, stdout);
which would avoid this problem.

Even though it doesn't seem like printf should ever change anything,
once printf runs off the end of one of your 128 byte buffers the
resulting undefined behavior could mess up malloc's internal control
tables causing a subsequent free() to fail.
printf("current->lenght=%d\n",current->size);
current = (*current).next;
}
printf((*current).data);
While not incorrect, your use of "(*current)." requires more typing
and is idiomatically more obscure than the conventional "current->".
printf("current->lenght=%d\n",current->size);
}
void reverseFile(struct buffer *first){
printf("frst->lenght=%d\n",first->size);
struct buffer *current = first;
struct buffer *previousElm = NULL;
int laatsteElm;
int i;
char *kopie;

while(1){
if(current==NULL){
break;
}
while(current->next != NULL){ //laatste buffer zoeken
previousElm=current;
current=current->next;
printf("\na\n");
Did you really mean to print a bunch of double spaced lines, each with
the same single character?
}
After you have executed the while(1) loop n-1 times, there is only one
node left. current, first, and previousElm all point to it. The
above while loop never executes because current->next is NULL. Ten
lines down you free it but then you come back to this point and ten
lines later try to free it again. You cannot free the same area
twice.
printf("\ncurrent->lng=%d\n",current->size);
printf("prev=%d\n",previousElm);
previousELm is a pointer. Why are you printing out its value, which
would be an address of no significance? Furthermore, %d requires an
int value which previousElm is not. The only portable way to print a
pointer is with %p and you must cast the value to void*.
laatsteElm = current->size;
kopie=(current->data)+(current->size)-1;
for(i=0;i < laatsteElm;i++){
printf("%c",*kopie);
kopie--;
}
free(current->data);
free(current);
current=first;
if(previousElm==NULL){
Can this ever be true?
return;
}
previousElm->next=NULL;

}
}

-------------------------------------------------------------------

<<Remove the del for email>>

[Zara]

On 4 Oct 2005 11:38:34 -0700, "Henk" <cc*****@hotmail.com> wrote:

Thanks for the good explanation, it really helped.
Hope to learn from my mistakes and get better.

Greetings Henk

Just two notes:
One: It is considered good politics to quote the relevant parts of the
message so that an accidental reader might now where all comes from.

Two: I think I have seen the same problem statement of your original
post as a question in other posting (from other person). This other
person just wanted the full solution. You have been working a solution
and, when it did not work in spite of all tyour tries, you asked for a
limited, well oriente help. Well I *love* when newbiesdo this, that
means they want to be programmers. Welcome.

[Michael Wojcik]

In article <43***************@news.xs4all.nl>, rl*@hoekstra-uitgeverij.nl (Richard Bos) writes:

"Henk" <cc*****@hotmail.com> wrote:

void reverseFile(struct buffer *first){
printf("frst->lenght=%d\n",first->size);
I neglected to mention that in C90 variable declarations must preceed
statements in a block, so this printf would have to be moved after
the next two lines. In C99, declarations and statements can be
mixed, and many pre-C99 compilers provide this as an extension, but
it's a good idea to avoid it if you want portable code.
struct buffer *current = first;
struct buffer *previousElm = NULL;

...

while(current->next != NULL){ //laatste buffer zoeken
previousElm=current;
current=current->next;
printf("\na\n");
}
Note that this loop does not set previousElm if you have a one-node list
(i.e., when current->next starts out null). Since this function breaks
down the list, you will eventually encounter this situation. At that
point, either previousElm will contain garbage (if first->next, as
passed to the function, is null);

Actually, in this case previousElm will be NULL, as it's initialized
in the declaration above. The code should work if the original list
is only one element long.
or it will contain the value from the
previous loop, which is then the same as current.

But if the original list has two or more elements, then previousElm
will indeed have a stale value.

--
Michael Wojcik mi************@microfocus.com

I would never understand our engineer. But is there anything in this world
that *isn't* made out of words? -- Tawada Yoko (trans. Margaret Mitsutani)

[Henk]

Hi Guy's,

I worked on (almost) all the errors, but I am still getting a
segmentation fault after running my program. We I just use 1 buffer
everything seems to be going fine. But with more the one buffer (the
program works...it reverses my file) but it ends u with giving me a
"segmentation error"

The problem must be within this piece of code:

void reverseFile(struct buffer *first){

struct buffer *current = first;
struct buffer *previousElm = NULL;

int laatsteElm;
int i;
char *kopie;

while(1){
if(current==NULL){
break;
}
while(current->next != NULL){ //laatste buffer zoeken
previousElm=current;
current=current->next;
}

laatsteElm = current->size;
kopie=(current->data)+(current->size)-1;
for(i=0;i < laatsteElm;i++){
printf("%c",*kopie);
kopie--;
}
free(current->data);
free(current);
current=first;
if(previousElm==NULL){
return;
}
previousElm->next=NULL;

}

}

Does anybody know what the problem is? I am getting a little bit
creazy, I have been starring at my code for the last 3 hours, and did
not know what the problem was.

Greetings Henk

[Michael Wojcik]

In article <11*********************@g44g2000cwa.googlegroups. com>, "Henk" <cc*****@hotmail.com> writes:

I worked on (almost) all the errors, but I am still getting a
segmentation fault after running my program. We I just use 1 buffer
everything seems to be going fine. But with more the one buffer (the
program works...it reverses my file) but it ends u with giving me a
"segmentation error"
Yes, that's one possible result of undefined behavior.
The problem must be within this piece of code:
It's the same problem it had in the previous version of the program,
and the same advice applies: look at what happens to previousElm if
the list contains two elements.
I have been starring at my code for the last 3 hours, and did
not know what the problem was.

Staring at code is rarely an effective debugging technique. To
find a bug, you need to find which of your assumptions is wrong,
and the way to do that is to test them.

Here are some options:

1. Use a debugger. Step through the function while it runs. Look
at the values of the current and previousElm pointers, and at what
values you pass to free.

2. Take a piece of paper. Make a diagram of a two-element list,
like this:

+---+ +---+
| | --> | | --> null
+---+ +---+
^
|
first

Take a couple of tokens - two distinct coins, perhaps - and call one
"current" and one "previousElm". Looking at a listing of your
function, walk through your function, moving the tokens to correspond
to what they point to.

When you free something, draw an X through it so you know it's been
freed and that address is no longer valid. When you change a list
pointer (eg "previousElm->next = NULL"), draw a new arrow in your
diagram.

You could also assign your own "addresses" to the list elements -
just number them - and keep a list of which addresses have been
passed to free on your paper.

(Actually, you might want to try a three- or four-element list with
this method, so that you can see what happens in both normal and
boundary cases. The two-element case will show you your bug, if
you do it correctly, but it won't show you why it doesn't happen on
every iteration. That's useful to know when you go to fix it.)

--
Michael Wojcik mi************@microfocus.com

If Mokona means for us to eat this, I, a gentle person, will become
angry! -- Umi (CLAMP & unknown translator), _Magic Knight Rayearth_

[Henk]

Ok, just call me plain old stupid....but I really cannot see the
mistake.
I made a diagram and put assert() in the code to check the values of
current and previousElm and if they are NULL when they are supposed to
be. I feel really stupid but I just don't see it.
Could you tell me what I did wrong. I know for sure that I will learn
from my mistake.

Greetings Henk

[Barry Schwarz]

On 6 Oct 2005 07:09:02 -0700, "Henk" <cc*****@hotmail.com> wrote:

Hi Guy's,

I worked on (almost) all the errors, but I am still getting a
segmentation fault after running my program. We I just use 1 buffer
everything seems to be going fine. But with more the one buffer (the
program works...it reverses my file) but it ends u with giving me a
"segmentation error"

The problem must be within this piece of code:
If you would indent a reasonable amount, it would probably be obvious.
Indenting and bracing adjusted; view in a monospaced font.

Let's assume your linked list consists of two nodes. Let's assume the
first is at location 100 and the second at location 200.

void reverseFile(struct buffer *first)
{

struct buffer *current = first;
Both current and first contain 100.
struct buffer *previousElm = NULL;
previousElm contains 0.

int laatsteElm;
int i;
char *kopie;

while(1)
{
Iterations refer to the while loop.
if(current==NULL)
{
On the first iteration, current is 100 (not NULL).
Same for second iteration.
Same for third iteration.
break;
Not executed on iteration 1.
Same for iteration 2.
Same for iteration 3.
}
while(current->next != NULL)
True on iteration 1.
False on iteration 2.
False on iteration 3.
{ //laatste buffer zoeken
previousElm=current;
On iteration 1 previousElm is assigned 100.
current=current->next;
On iteration 1 current is assigned 200.
}
Not executed on iteration 2. current and previousElm retain their
value of 100.
Same for iteration 3.

laatsteElm = current->size;
On iteration 3, this invokes undefined behavior. You are
dereferencing current (100) which points to an area that has already
been freed.
kopie=(current->data)+(current->size)-1;
Ditto.
for(i=0;i < laatsteElm;i++)
{
printf("%c",*kopie);
Ditto.
kopie--;
}
free(current->data);
free(current);
On iteration 1, 200 and 200->data are freed.
On iteration 2, 100 and 100->data are freed.
On iteration 3, you try to free 100 and 100->data again. More
undefined behavior and the most likely place for a seg fault. This is
the same problem I told you about in the my last message.
current=first;
On iteration 1, current is assigned 100.
Same for iteration 2.
I doubt if you get here for iteration 3.
if(previousElm==NULL)
On iteration 1 this is false since previousElm is 100.
Same for iteration 2.
{
return;
Not executed on iteration 1.
Same for iteration 2.
}
previousElm->next=NULL;

Perform another iteration.
}

}

Does anybody know what the problem is? I am getting a little bit
creazy, I have been starring at my code for the last 3 hours, and did
not know what the problem was.

Greetings Henk

<<Remove the del for email>>

[Michael Wojcik]

In article <11**********************@z14g2000cwz.googlegroups .com>, "Henk" <cc*****@hotmail.com> writes:

Ok, just call me plain old stupid....but I really cannot see the
mistake.
I made a diagram ...
I suspect the problem is that you're taking shortcuts when you
desk-check the code.

Don't just make a diagram. Make the diagram, then *walk through the
code*. Line by line. Update the diagram and the values you've
written down after every statement. Every time you dereference a
pointer, check to see that it's valid - that it's not null, that it
points to memory you've allocated, that the location hasn't been
freed. Every time you call free, check that the argument is valid.

If you make a diagram and then guess what the code will do with it,
you're just relying on your assumptions again. You have to actually
do what the code does, on paper. Be a C implementation.
and put assert() in the code

No, no, no. You haven't finished with the diagram yet. You're
taking shortcuts that validate your assumptions. While assertions
can be a way to find bugs (though I dislike them myself), you're
using them as an excuse to not desk-check thoroughly.

I see Barry Schwarz has posted a detailed description of the
problem, so I won't. Basically, if your list starts off with two
or more elements, then when it gets down to one element previousElm
won't be updated anymore and you'll try to operate on nodes that
you've already freed.

--
Michael Wojcik mi************@microfocus.com

Pocket #9: A complete "artificial glen" with rocks, and artificial moon,
and forester's station. Excellent for achieving the effect of the
sublime without going out-of-doors. -- Joe Green

[Dave Thompson]

On 4 Oct 2005 17:11:39 GMT, mw*****@newsguy.com (Michael Wojcik)
wrote:

In article <11**********************@f14g2000cwb.googlegroups .com>, "Henk" <cc*****@hotmail.com> writes: <snip>

struct buffer* createBuffer(){

<snip> Also, note that it's widely considered poor style to separate the
"*" and the identifier for an identifier to pointer type. It leads
to errors such as:

int* ptrA, ptrB;

where, contrary to its name, "ptrB" is actually an int, not an int
pointer.

Beg to differ slightly. I agree it's usually a bad idea to join the *
with the type-specifiers, but that doesn't mean it should always be
joined to the name instead. E.g. I like:

int * ary_of_ptrs[10];
float * func_ret_ptr(argtypes);
/* or even */
float * func_ret_ptr (argtypes);

- David.Thompson1 at worldnet.att.net

[Michael Wojcik]

In article <02********************************@4ax.com>, Dave Thompson <da*************@worldnet.att.net> writes:

On 4 Oct 2005 17:11:39 GMT, mw*****@newsguy.com (Michael Wojcik)
wrote:

Also, note that it's widely considered poor style to separate the
"*" and the identifier for an identifier to pointer type.

Beg to differ slightly. I agree it's usually a bad idea to join the *
with the type-specifiers, but that doesn't mean it should always be
joined to the name instead. E.g. I like:

int * ary_of_ptrs[10];
float * func_ret_ptr(argtypes);

I take your point. In a declaration like these for an identifer that
begins "type * identifier" (perhaps with storage class and/or
qualifier), but "*identifier" isn't an object of "type", separating
the "*" and the identifer can document that fact.

So:

float (*func_ptr)(argtypes);

for a pointer to a function returning float, but

float * func_ret_ptr(argtypes);

for a function returning a float pointer. That could indeed make the
code clearer on casual inspection (which is the main motivation for
whitespace, after all).

--
Michael Wojcik mi************@microfocus.com

Push up the bottom with your finger, it will puffy and makes stand up.
-- instructions for "swan" from an origami kit