Newbie questions - Help! 
Author Message
 Newbie questions - Help!

Below is a C programme that reads an arbitrary number of decimals from
a file (in ASCII format, each separated by a newline) reverses their
order and prints them. The programme works OK, but since I'm a newbie,
I feel I've messed things up with pointers and memory allocation. So
can anyone comment on the questions in the programme and anything in
general? Thanks!

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

int *readFile(char *);
void reverseArray(const int *, int *, int);

void main(int argc, char *argv[]) {
        int *temp, i=0;

        if(argc == 1) { printf("Enter the file you want to process\n");
return;}
        temp = readFile(argv[1]);

        printf("---------REVERSED ARRAY----------\n");

        while( *(temp + i) != -999) {
                printf("%d\n", *(temp+i));
                i++;
        }

Quote:
}

int *readFile(char *filename)
{
        FILE *file;
        int *numbers, temp=0, tmp=0;
        int MAX=100;

        numbers = (int *) malloc(200 * sizeof(int));

        if( (file = fopen(filename, "r")) == NULL) {
                printf("Cannot open file!\n");
                exit(2);
        }

        // Read the first number off the file and store it
        // in the first position of the array.
        temp = fscanf(file,"%d",&tmp);
        *numbers = tmp;

        int i=1;        // Counter holding number of elements in file.

        while(temp != EOF) {
                // Read next number and store it in array.
                temp = fscanf(file, "%d", numbers+i);

                i++;    // Increment counter.

                // Is this right?
                if(i==MAX) {
                        printf("Enlarging array");
                        realloc(numbers, sizeof(int) * 100);
                        i=0;
                }
        }

        fclose(file);

        // Does this free up any space not used?
        // I.e. Memory reserved was 200 bytes,
        // will this free up 200 - i bytes?
        realloc(numbers, sizeof(int) * i-1);

        int *result;    // Pointer to reversed array.

        reverseArray(numbers,result, i-1);

        free(numbers);  // Free up memory.

        // Now I get confused. Where does result point to
        // now? It hasn't reserved any memory, so where does
        // it point to? Why does it contain the correct result
        // if you print it out?
        // If I free result, how can I return it to the calling
        // function afterwards? Is this right?
        return result;

Quote:
}

void reverseArray(const int *array, int *result, int size)
{
        int count = size;

        for(int i=0; i < count; i++, size--)
                *(result + i) = *(array + size-1);

        *(result + i) = -999;   // Add a sentinel to indicate
                                // end of array.

Quote:
}



Tue, 08 Feb 2005 20:07:37 GMT  
 Newbie questions - Help!
Quote:

> Below is a C programme that reads an arbitrary number of decimals from
> a file (in ASCII format, each separated by a newline) reverses their
> order and prints them. The programme works OK, but since I'm a newbie,
> I feel I've messed things up with pointers and memory allocation. So
> can anyone comment on the questions in the programme and anything in
> general? Thanks!

You are (un)lucky if it works as expected as there are a few places
where you invoke undefined behavior.  See comments below.

Quote:
> #include <stdio.h>
> #include <stdlib.h>

> int *readFile(char *);
> void reverseArray(const int *, int *, int);

> void main(int argc, char *argv[]) {

int main(int argc, char *argv[]) {
main returns int in C.  If you have a C book that shows code with main
returning anything other than int, throw it away and get a better one.

Quote:
>    int *temp, i=0;

>    if(argc == 1) { printf("Enter the file you want to process\n");
> return;}

return 0;
Remember, main returns int.

Quote:
>    temp = readFile(argv[1]);

>    printf("---------REVERSED ARRAY----------\n");

>    while( *(temp + i) != -999) {
>            printf("%d\n", *(temp+i));
>            i++;
>    }

return 0;
main returns int.

Quote:
> }

> int *readFile(char *filename)
> {
>    FILE *file;
>    int *numbers, temp=0, tmp=0;
>    int MAX=100;

>    numbers = (int *) malloc(200 * sizeof(int));

Check the return of malloc.  malloc returns NULL if it fails.  It's a
good practice to learn to do this reflexively.

Quote:

>    if( (file = fopen(filename, "r")) == NULL) {
>            printf("Cannot open file!\n");
>            exit(2);

Some may carp that you should use EXIT_FAILURE here instead of the
hard-coded 2.

Quote:
>    }

>    // Read the first number off the file and store it
>    // in the first position of the array.
>    temp = fscanf(file,"%d",&tmp);

What happens if this was an empty file?
Quote:
>    *numbers = tmp;

>    int i=1;        // Counter holding number of elements in file.

>    while(temp != EOF) {

You can re-write the above as:
         while ( (temp = fscanf(file, "%d", &tmp)) != EOF ) {
and avoid the unnecessary fscanf outside of the loop.  You would have to
change the i=1 initialization to i=0 instead.

Quote:
>            // Read next number and store it in array.
>            temp = fscanf(file, "%d", numbers+i);

>            i++;    // Increment counter.

>            // Is this right?
>            if(i==MAX) {

MAX is not been defined.

Quote:
>                    printf("Enlarging array");
>                    realloc(numbers, sizeof(int) * 100);

Read the description of realloc.  It reallocates and returns a pointer
to the new region.  Since you have simply thrown this value away,
numbers may now be an invalid pointer.  More importantly, you need to
give realloc the new size for the old region.  In your case, you need to
expand the existing region.  If you hard-code a value for the size as
you have done, this could lead to a smaller sized memory than the
original.  Not quite what you want, is it?  You should keep track of the
current size of the region and keep expanding it by some increment on
top of the current size.

One way to do the realloc is:
            int  cursz = 200;
            numbers = malloc(sizeof *numbers * cursz);
                ...
            int *tempptr;
            tempptr = realloc(numbers, sizeof *numbers *(cursz + 200);
            if ( !tempptr ) /* error in realloc */...
            else {
                numbers  = tempptr;
                cursz   += 200;
            }

Quote:
>                    i=0;

Huh??  You will start over-writing all those numbers that you read into
numbers by resetting i to 0.  Get rid of this.

Quote:
>            }
>    }

>    fclose(file);

>    // Does this free up any space not used?
>    // I.e. Memory reserved was 200 bytes,
>    // will this free up 200 - i bytes?
>    realloc(numbers, sizeof(int) * i-1);

Again, don't throw away the return of realloc.

Quote:

>    int *result;    // Pointer to reversed array.

result is an uninitialized pointer.  de-referencing it or using it for
any computational purpose leads to undefined behavior.  You should
allocate memory for result using malloc just as you did for numbers.
How much?  Enough to hold i integers (i-1 data and 1 for your sentinel).

Quote:
>    reverseArray(numbers,result, i-1);

>    free(numbers);  // Free up memory.

Huh??  Your array numbers that you went into great trouble to read the
integers from the file and then reverse them is now gone. Using numbers
after this statement causes undefined behavior.

Quote:

>    // Now I get confused. Where does result point to
>    // now? It hasn't reserved any memory, so where does
>    // it point to? Why does it contain the correct result
>    // if you print it out?
>    // If I free result, how can I return it to the calling
>    // function afterwards? Is this right?
>    return result;

result pointed to garbage.  still points to garbage.

Quote:
> }

> void reverseArray(const int *array, int *result, int size)

result being passed in is garbage.  this is already results in undefined
behavior (pun unintended).  Fix this by allocating result before passing
it in.

Quote:
> {
>    int count = size;

>    for(int i=0; i < count; i++, size--)

This isn't C.  You can declare i inside the for statement in C.  You
would have to declare i before you use it here.

Quote:
>            *(result + i) = *(array + size-1);

>    *(result + i) = -999;   // Add a sentinel to indicate
>                            // end of array.
> }

I am not sure I caught all the problems, but atleast that should give
you enough to work on.  Some thoughts on the implementation:
- What if -999 is part of your input?
- You can reverse an array in-place without using an entire duplicate
array to store the result.

HTH,
nrk.



Tue, 08 Feb 2005 21:49:24 GMT  
 Newbie questions - Help!

Quote:
>Below is a C programme that reads an arbitrary number of decimals from
>a file (in ASCII format, each separated by a newline) reverses their
>order and prints them. The programme works OK, but since I'm a newbie,

If this works as intended, the gods have indeed looked upon you with
uncharacteristic favor.  :)

See notes below.

Quote:
>#include <stdio.h>
>#include <stdlib.h>

>int *readFile(char *);
>void reverseArray(const int *, int *, int);

>void main(int argc, char *argv[]) {

main() returns an int--this is standard-defined behavior.

Change this to

        int main(int argc, char *argv[])

Quote:
>    int *temp, i=0;

>    if(argc == 1) { printf("Enter the file you want to process\n");
>return;}

Somewhat nit-pickish:  If you return a value, it should be well
characterized and have meaning.  In this case, I'd suggest returning
EXIT_FAILURE to indicate that things are wrong.

Quote:
>    temp = readFile(argv[1]);

Later in your code, you use a sentinel value of -999 to indicate the end of
the array read by this function.  You could easily rewrite readFile() to
return the number of elements in your array, thus eliminating the need to
use a sentinel.  Besides, what if -999 was actually part of your data?

Here's a prototype for your revised function:

        int *readFile(const char *filename, size_t *cnt);

where 'cnt' is a pointer to a size_t value that will hold the number of
integers you've read from the 'filename' file.  readFile() will return a
pointer to the first element of your int array or NULL if an error
occurred.

Since you now know the length of your array, you can rewrite reverseArray
as

        void reverseArray(int *array, size_t cnt);

An aside:  I've replaced int with size_t when referencing array subscripts.
size_t is guaranteed to be of an unsigned integral type large enough to
reference the size of the largest data object possible on your
implementation; int is not.

More notes on reverseArray() later on.

Quote:
>int *readFile(char *filename)
>{
>    FILE *file;
>    int *numbers, temp=0, tmp=0;
>    int MAX=100;

>    numbers = (int *) malloc(200 * sizeof(int));

What if malloc() fails?  Always check the return value.

Also, it would be better to put the size of the array (initially 200) into
a variable and keep it updated.  When you reallocate the array, you would
typically want to increment the current size by some constant value:

        #define ARRAYBLK        100

        int *numbers, *p;
        size_t csiz = 200;

        if ((numbers = (int *) malloc(csiz * sizeof(int))) == NULL)
        {
                /* error handling */
        }

        /* ... */

        /* Need to realloc */

        csiz += ARRAYBLK;

        if ((p = realloc(numbers, csiz * sizeof(int))) == NULL)
        {
                /* error handling */
        }

        numbers = p;    /* Make sure numbers points to current loc. of array */

Quote:
>    if( (file = fopen(filename, "r")) == NULL) {
>            printf("Cannot open file!\n");
>            exit(2);

See above comments regarding meaningful exit codes.

Quote:
>    }

>    // Read the first number off the file and store it
>    // in the first position of the array.
>    temp = fscanf(file,"%d",&tmp);
>    *numbers = tmp;

>    int i=1;        // Counter holding number of elements in file.

You cannot declare an object imbedded within a block of code like this in
C, although you can in C++.  You'll need to either move this declaration to
the top of the current block (before any statements), or encapsulate this
section within another set of braces.

If you're writing C code, you may want to turn off your compiler's C++ mode
extensions, at least until you are more familiar with the language.

Quote:

>    while(temp != EOF) {
>            // Read next number and store it in array.
>            temp = fscanf(file, "%d", numbers+i);

>            i++;    // Increment counter.

>            // Is this right?
>            if(i==MAX) {
>                    printf("Enlarging array");
>                    realloc(numbers, sizeof(int) * 100);
>                    i=0;
>            }
>    }

You could vastly simplify this (and avoid confusion) by testing for EOF as
you read the data:

        i = 0;

        while ((temp = fscanf(file, "%d", &tmp)) != EOF)
        {
                numbers[i] = tmp;
                i++;

                /* ... */
        }

Quote:
>    fclose(file);

>    // Does this free up any space not used?
>    // I.e. Memory reserved was 200 bytes,
>    // will this free up 200 - i bytes?
>    realloc(numbers, sizeof(int) * i-1);

You've just lost your numbers array.  Further dereferences on the 'numbers'
pointer will yield unspecified behavior.

realloc() returns a pointer to the reallocated array, and you've discarded
that return value.

Furthermore, you don't need to resize the array to hold just enough for the
input data.  This just adds to the complexity without buying you very much,
if anything at all.  At most, you're wasting ARRAYBLK * sizeof(int) bytes,
which, since you requested and were granted them, had nothing better to do
anyway. :-)

Quote:

>    int *result;    // Pointer to reversed array.

>    reverseArray(numbers,result, i-1);

I'd move this out of the readFile() function.  readFile() should just read
the data and populate the array, at most.

See notes regarding reverseArray() further down.

Quote:
>    free(numbers);  // Free up memory.

>    // Now I get confused. Where does result point to
>    // now? It hasn't reserved any memory, so where does

You've freed an unspecified region of memory here--recall the "resize to
fit" final reallocation you did?

As for result, it was an uninitialized pointer.  Your reverseArray()
function happily used it anyway, with unspecified results (har, har).

Quote:
>    // it point to? Why does it contain the correct result
>    // if you print it out?

Unspecified means all bets are off.  If you're lucky, you'll get a
particularly impressive system crash.  OTOH, you might just happen to get
the results you expected, but that's just the universe playing a mean trick
on you.

Quote:
>    // If I free result, how can I return it to the calling
>    // function afterwards? Is this right?
>    return result;
>}

Unspecified behavior.

Reconsider what you want to do.  readFile() is supposed to read the input
file and return an array of its contents, yes?  Then why free the array
you've spent so much effort constructing before you even get to use it?

Let the calling function (main) worry about freeing the array after its
done with it.

Quote:
>void reverseArray(const int *array, int *result, int size)
>{
>    int count = size;

>    for(int i=0; i < count; i++, size--)

See above comments regarding imbedded declarations in a statement block.

Quote:
>            *(result + i) = *(array + size-1);

>    *(result + i) = -999;   // Add a sentinel to indicate
>                            // end of array.
>}

Wouldn't it be much simpler to reverse the array in place, rather than
copying it to another array?  What if -999 were a legitimate part of your
data?  Consider this:

Use a head and tail subscript, e.g., initially head == 0 and tail ==size-1.
Swap the head array element (numbers[head]) for the tail element.
Increment the head, and decrement the tail, e.g., now head == 1 and tail ==
size-2.
Wash, rinse and repeat.

How do you know when you're done?

Give your code another pass, and consider the revisions I've suggested.
Hope this helps!

--
Robert B. Clark (email ROT13'ed)
Visit ClarkWehyr Enterprises On-Line at http://www.3clarks.com/ClarkWehyr/



Wed, 09 Feb 2005 02:32:03 GMT  
 Newbie questions - Help!

Quote:


>>        for(int i=0; i < count; i++, size--)
> This isn't C.  You can declare i inside the for statement in C.  You
> would have to declare i before you use it here.

Not 100% true --> in C99 it's okay.  But, it should be avoided in
code that is intended to be portable (at least for a while).


Wed, 09 Feb 2005 20:00:39 GMT  
 Newbie questions - Help!
On Fri, 23 Aug 2002 13:32:03 -0500, Robert B. Clark

Quote:

>>        // Read the first number off the file and store it
>>        // in the first position of the array.
>>        temp = fscanf(file,"%d",&tmp);
>>        *numbers = tmp;

>>        int i=1;        // Counter holding number of elements in file.

>You cannot declare an object imbedded within a block of code like this in
>C, although you can in C++.  You'll need to either move this declaration to
>the top of the current block (before any statements), or encapsulate this
>section within another set of braces.

You can in C99, but not C89/90. Odds are that the original poster is
using a C90 compiler, though.

-Kevin



Thu, 10 Feb 2005 02:29:26 GMT  
 Newbie questions - Help!
I would like to thank everyone for their responses! They helped me out
quite a lot...Indeed I must be pretty lucky, since it still works
fine!

One more question though...What is wrong with having an uninitialized
pointer?

I.e.

int *num;

int number = 5;

num = &number;

Why is this right?

And int *num;

for(i=0; i < 10; i++)
    *(num + i) = *(other + i)  <== Why is this wrong?

My point: When do I have to allocate memory for a pointer?

2nd: Why does this not work in C? (it does in C++)

*num=5
num++;
*num = 6;

From my programme, it didn't store values properly. Instead *(num + i)
= 3 works fine.

Finally, is it Ok to initialise a pointer to 0? i.e. int *result = 0;

Thanks again



Sat, 12 Feb 2005 04:48:27 GMT  
 Newbie questions - Help!

Quote:

> I would like to thank everyone for their responses! They helped me out
> quite a lot...Indeed I must be pretty lucky, since it still works
> fine!

> One more question though...What is wrong with having an uninitialized
> pointer?

If a pointer isn't pointing anywhere useful, there's no benefit in
dereferencing it; there *is*, however, potential *harm* in dereferencing
it.

Quote:

> I.e.

> int *num;

> int number = 5;

> num = &number;

> Why is this right?

num is assigned the address of an int. That's what pointers to int are
for, so everything is fine.

Quote:

> And int *num;

> for(i=0; i < 10; i++)
>     *(num + i) = *(other + i)  <== Why is this wrong?

num doesn't point anywhere useful. Thus, num + i is meaningless, and
*(num + i) even more meaningless.

Quote:

> My point: When do I have to allocate memory for a pointer?

Before you can dereference a pointer, you must point it somewhere. You
might point it at an existing object, in which case you don't have to
allocate memory for it to point at because the memory already exists
(and is occupied by the object to which the pointer points).

If you want somewhere new for it to point at, so that you can copy data
into that space, you can do so by using the malloc() function:

#include <stdlib.h>

int main(void)
{
  int *p = malloc(sizeof *p);
  if(p != NULL)
  {
    *p = 42; /* valid */
    free(p); /* finished with this memory now, so we release it */
  }
  return 0;

Quote:
}

> 2nd: Why does this not work in C? (it does in C++)

> *num=5
> num++;
> *num = 6;

If num points to a single int, then *num = 5; is fine. num++ is legal
for technical reasons that I don't want to burden you with just now, but
it no longer points anywhere useful. *num = 6 is therefore an error -
the technical term is "undefined behaviour", and the outcome can, in
theory at least, be *anything*, including the possibility that the code
does what you expected it to do (but by no means limited to that).

This is illegal in C++, too - you were just unlucky when it did what you
expected. Why unlucky? Because that behaviour has led you to believe
that the code is legal in C++, which it isn't.

Quote:

> From my programme, it didn't store values properly. Instead *(num + i)
> = 3 works fine.

> Finally, is it Ok to initialise a pointer to 0? i.e. int *result = 0;

Yes. A pointer with a value of 0 is guaranteed not to point to any valid
object, so it's quite handy for "parking" pointers that you know you
need but that you don't want to point anywhere legal just at the moment.

--

"Usenet is a strange place." - Dennis M Ritchie, 29 July 1999.
C FAQ: http://www.eskimo.com/~scs/C-faq/top.html
K&R answers, C books, etc: http://users.powernet.co.uk/eton



Sat, 12 Feb 2005 06:41:33 GMT  
 
 [ 7 post ] 

 Relevant Pages 

1. Newbie Question Help

2. Newbie Questions - Help Please!

3. Newbie question: Help with string handling/conversion

4. Newbie question: Need help please!

5. getchar() : newbie question, pls HELP

6. Newbie Question, please help with searching

7. Need help in C- newbie question

8. Please Help! --newbie question, very simplistic stuff--

9. Help: initialization (Newbie Question)

10. Newbie question: Need help please!

11. Random Variable help! (newbie question)

12. Newbie question: Need help finding a particular C keyword

 

 
Powered by phpBB® Forum Software