In article <1128424500.177985.271500@f14g2000cwb.googlegroups .com>, "Henk" <cccprog@hotmail.com> writes:[color=blue]
>
> stack.c: In function 'reverseFile'
> stack.c:98: warning:int format, pointer arg (arg 2)[/color]
This is the line in question:
[color=blue]
> printf("prev=%d\n",previousElm);[/color]
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 *:
[color=blue]
> printf("prev=%p\n", (void *)previousElm);[/color]
[color=blue]
> 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![/color]
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:
[color=blue]
> while( !(feof(file))){ //VERANDERD[/color]
Don't do this; test the result of fread. See the comp.lang.c FAQ.
[color=blue]
> fread(current->data,(sizeof(char)),BUFFER_SIZE,file);[/color]
sizeof(char) is always 1. And why the extra parentheses around it?
[color=blue]
> clearerr(file);
> fclose(file);[/color]
There's no point in calling clearerr on a FILE* that you're about
to fclose.
[color=blue]
> struct buffer* createBuffer(){[/color]
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.
[color=blue]
> buf=malloc(sizeof(struct buffer));[/color]
buf = malloc(sizeof *buf);
is better; if the type of buf changes, the argument to malloc will
still be correct.
[color=blue]
> buf->data = malloc(sizeof(char)*BUFFER_SIZE);[/color]
Again, sizeof(char) is always 1.
[color=blue]
> while((*current).next != NULL){[/color]
(*current).next is the same as current->next. That's the whole
point of the arrow operator.
[color=blue]
> printf((*current).data);[/color]
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.
[color=blue]
> while(1){
> if(current==NULL){
> break;
> }[/color]
while (current != NULL){
has the same semantics and is clearer. However, you have another
loop invariant buried in the loop:
[color=blue]
> if(previousElm==NULL){
> return;
> }[/color]
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.
[color=blue]
> int main(int argc, char** argv){
>
> char *filename=argv[1];[/color]
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
michael.wojcik@microfocus.com
Please enjoy the stereo action fully that will surprise you. -- Pizzicato Five
