Need help with my C code please.

[zxz]

hi everyone , i have worked on this 2 days now and cant find my mistakes , the program runs but it stops ( waiting ) after i enter 3 or 4 . can anyone help me where are my mistakes please? thanks allot.

the program should do the following :

user will be asked how many numbers he wants to enter ( 3 or 4 ).
3 or 4 integer values will be entered from user and saved inside an array.
the program counts how many zeros the user entered.
the program then doubles the array to the double size , where each element has a copy of it self next to it.
print out the new array at the end.

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

static int cnt = 0;


/* Read <cnt> Integers into given array.
   Return number of entered zeros */
int* readInts(int *arr) {
    int i=0, *zeros = (int*)malloc(sizeof(int));

    while(i <= cnt) {
        scanf("%d", arr++);
        if (arr[i++] == 0) *zeros++;
    }
    return zeros;
}

/* Create "double-sized" copy of array (duplicate each value)*/
void extend(int *arr, int *newarr) {
    int i,j;
    newarr = (int*) malloc(2 * cnt * sizeof(int*));
    for (i=0,j=0; i <= cnt; i++) {
        newarr[j++] = arr[i];
        newarr[j++] = arr[i];
    }
}

int main(void) 
{
    int arr[4], *zeros, i;
    printf("How many integers (3 or 4)?\n");
    scanf("%d", &cnt);

    zeros = readInts(arr); 
    printf("You entered %d zero(s)!\n", *zeros);

    int *newarr;
    extend(arr, newarr);

    for (i=0; i < cnt*2; i++) printf("%d ", newarr[i]);
    printf("\n");

    return 0;
}

[deceptikon]

There are a few problems, and most of them stem from misunderstanding pointers.

On line 12 you have this:

while(i <= cnt)

That's an off by one error. Arrays are 0-based in C, which means if the user enters 3, you're processing the indexes 0, 1, 2, and 3 rather than just 0, 1, and 2. It should simply be while (i < cnt).

if (arr[i++] == 0) *zeros++;

This is the first pointer related problem. *zeros++ will dereference zeros, then increment the pointer, not the pointed to value which is what you really want. Precedence and order of operations can be a little confusing when pointers are involved, but you can do either (*zeros)++ or switch to prefix increment with ++*zeros to get the correct behavior. Also note that you never initialized *zeros to 0, so its value is garbage.

The zero check also contains an off by one error in that you neglect to check the first value in arr by both incrementing arr during input, which is prior to the zero check.

I'd also question what the point of making zeros a pointer here as you could just use a straight up int and return that rather than mucking about with malloc and a pointer.

Finally, the global cnt smells bad. You should avoid global dependencies whenever possible, which in this case it most certainly possible by passing the size of the array as a parameter:

int readInts(int *arr, int cnt)
{
    int i = 0, zeros = 0;

    while (i < cnt)
    {
        scanf("%d", &arr[i]);

        if (arr[i++] == 0)
        {
            ++zeros;
        }
    }

    return zeros;
}

extend also contains an off by one error in the loop, but the larger problem is you're leaking memory. newarr is a copy of the object passed in, which means any change you make directly to the pointer doesn't get mirrored back in main. There are two options to fix that. First, if you cannot use the return value, simply make the parameter a pointer to a pointer and pass the address of the original pointer:

void extend(int *arr, int **newarr, int cnt)
{
    int i, j;

    *newarr = (int*)malloc(2 * cnt * sizeof(int*));

    for (i = 0, j = 0; i < cnt; i++)
    {
        (*newarr)[j++] = arr[i];
        (*newarr)[j++] = arr[i];
    }
}

...

extend(arr, &newarr, cnt);

This gives you access to the original object back in main, and all is well. However, this complicates a lot of things. A better approach if you can modify the return value (which you can in this case), is to create a local pointer and return it:

int *extend(int *arr, int cnt)
{
    int *newarr = (int*)malloc(2 * cnt * sizeof(int*));
    int i, j;

    for (i = 0, j = 0; i < cnt; i++)
    {
        newarr[j++] = arr[i];
        newarr[j++] = arr[i];
    }

    return newarr;
}

...

int *newarr = extend(arr, cnt);

With those changes you'd end up with something like this:

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

int readInts(int *arr, int cnt)
{
    int i = 0, zeros = 0;

    while (i < cnt)
    {
        scanf("%d", &arr[i]);

        if (arr[i++] == 0)
        {
            ++zeros;
        }
    }

    return zeros;
}

int *extend(int *arr, int cnt)
{
    int *newarr = malloc((2 * cnt) * sizeof *newarr);
    int i, j;

    for (i = 0, j = 0; i < cnt; i++)
    {
        newarr[j++] = arr[i];
        newarr[j++] = arr[i];
    }

    return newarr;
}

int main(void)
{
    int arr[4], zeros, i, cnt;
    int *newarr;

    printf("How many integers (3 or 4)?\n");
    scanf("%d", &cnt);

    printf("You entered %d zero(s)!\n", readInts(arr, cnt));

    newarr = extend(arr, cnt);

    for (i = 0; i < cnt * 2; i++)
    {
        printf("%d ", newarr[i]);
    }

    printf("\n");

    return 0;
}

[zxz]

thank you very much , the code now works perfectly :) .
i have used Valgrind to anylize the code but it couldnot find some memory accessing mistakes , can you please tell me which are those and why Valgrind couldnot find them?
many thanks in advance

[deceptikon]

can you please tell me which are those

newarr in main and newarr in extend are two completely independent variables. They contain the same value originally, but then you replace the value in extend with the result of malloc. That memory block is neither returned back to main nor freed, so it's a memory leak.

Worse, because it's not returned to main, newarr in main remains uninitialized and any attempt to dereference it is theoretically a segmentation fault.

why Valgrind couldnot find them?

Good question. Valgrind should at the very least be able to tell you that you make two calls to malloc yet there are no calls to free. My guess would be that it wasn't running memory checks.

[zxz]

ok thank you very much . that was a super help.
many thanks :)