Increment Array Size Failure

Question

I was trying to write a function to add primes to a array but my realloc kept failing. Can someone please look at my code and tell me why it is failing?

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

void makePrimes(int *primes, int *primesLength, int primeTarget);
int isPrime(int input);

int main(void)
{
    int *primes = (int *)(malloc(1));
    *primes = 2;
    int primesLength = 1;
    makePrimes(primes, &primesLength, 3);
    printf("%d", *(primes + 2));
    free(primes);
    return 0;
}

//* Finds the prime number that is the prime target number prime
//* (i.e 2 is the zeroth prime)
//! PRIME TARGET COUNTS FROM ZERO BUT  PRIMES LENGTH COUNTS FROM 1
void makePrimes(int *primes, int *primesLength, int primeTarget)
{
    // do nothing
    if (*primesLength >= primeTarget + 1)
        return;

    int lastValue = *(primes + (*primesLength - 1));
    while (primeTarget + 1 != *primesLength)
    {
        lastValue++;
        if (isPrime(lastValue))
        {
            (*primesLength)++;
            primes = realloc(primes, *primesLength * sizeof(int));
            *(primes + (*primesLength - 1));
        }
    }
}

//* Checks if a given integer is prime
int isPrime(int input)
{
    if (input == 0 || input == 1)
    {
        return 0;
    }
    int testFactor = 2;
    for (int iii = 0; iii < (input - 2); iii++)
    {
        if ((input % testFactor) == 0)
            return 0;
        testFactor++;
    }
    return 1;
}

I was expecting it to add one more available element to the array 'primes'. Using a debugger I found that primes was being set to NULL and not running the following line. Thanks.

Answer 1

1. You initially malloc() 1 byte instead of sizeof(int) == sizeof *primes which is 4 on my system.
2. Prefer sizeof variable instead of type. This means you don't have to repeat the type which makes the code less brittle in the face of future (type) changes.
3. Don't cast the void * from malloc().
4. As you want to change primes (with realloc()) you need to pass in &primes to makePrimes().
5. Use a temporary variable (tmp) for the return value of realloc() otherwise you leak memory on failure.
6. Check return value from malloc() and realloc().
7. Assign the value you found to primes.
8. (Not fixed) You are making it harder for yourself by insisting on using pointer instead of array notation.
9. (Not fixed) Consider using unsigned types. This eliminates error classes. For instance what if primeTarget is negative what is the expected output? What if a negative prime number is passed in?
10. isPrime() can be optimized to only look at testFactor <= input / testFactor.
11. (Not fixed) Consider reworking the interface so makePrimes() fills in the first value, too, instead of relying on caller to put in the right magic values for *primes and *primesLenght.
12. (Not fixed) As you specify how many primes you want, you could just pre-allocate the primes array instead of growing the array member at a time.
13. I made a printPrimes() function which is better for[debugging; you use your original code is just want the last one. If you just want the last prime why bother storing the rest in an array?

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

int isPrime(int input) {
    if (input == 0 || input == 1)
        return 0;
    for (int testFactor = 2; testFactor <= input / testFactor; testFactor++) {
        if (!(input % testFactor))
            return 0;
    }
    return 1;
}

void makePrimes(int **primes, int *primesLength, int primeTarget) {
    if(*primesLength < 1)
        return;
    int lastValue = *(*primes + (*primesLength - 1));
    while (*primesLength < primeTarget) {
        lastValue++;
        if (isPrime(lastValue)) {
            int *tmp = realloc(*primes, sizeof *tmp * (*primesLength + 1));
            if(!tmp) {
                printf("realloc failed\n");
                return;
            }
            *primes = tmp;
            *((*primes) + *primesLength) = lastValue;
            (*primesLength)++;
        }
    }
}

void printPrimes(int n, int primes[n]) {
    if(n <= 0) return;
    printf("%d", *primes);
    for(int i = 1; i < n; i++)
        printf(", %d", *(primes+i));
    printf("\n");
}

int main(void) {
    int *primes = malloc(sizeof *primes);
    if(!primes) {
       printf("malloc failed\n");
       return 1;
    }
    *primes = 2;
    int primesLength = 1;
    makePrimes(&primes, &primesLength, 3);
    printPrimes(primesLength, primes);
    free(primes);
    return 0;
}

example run:

2, 3, 5

Answer 2

in comments (text after //) there's why it was added or changed
upd: makePrimes' retvalue like realloc()

--- orig0.c
+++ orig1.c
@@ -1,16 +1,23 @@
 #include <stdio.h>
 #include <stdlib.h>
+#include <err.h>
 
-void makePrimes(int *primes, int *primesLength, int primeTarget);
+int* makePrimes(int *primes, int *primesLength, int primeTarget);
 int isPrime(int input);
 
 int main(void)
 {
-    int *primes = (int *)(malloc(1));
+    int *primes = (int *)(malloc(sizeof(int))); // allocate proper size
+    if (!primes) // test a result of malloc allocation
+        err(-1, "malloc");
     *primes = 2;
     int primesLength = 1;
-    makePrimes(primes, &primesLength, 3);
-    printf("%d", *(primes + 2));
+    primes = makePrimes(primes, &primesLength, 10);
+    if (primes) { // print all of them
+        for (int i = 0; i < primesLength; i++)
+            printf("%d ", primes[i]);
+        printf("\n");
+    }
     free(primes);
     return 0;
 }
@@ -18,11 +25,11 @@
 //* Finds the prime number that is the prime target number prime
 //* (i.e 2 is the zeroth prime)
 //! PRIME TARGET COUNTS FROM ZERO BUT  PRIMES LENGTH COUNTS FROM 1
-void makePrimes(int *primes, int *primesLength, int primeTarget)
+int* makePrimes(int *primes, int *primesLength, int primeTarget) // return pointer like realloc()
 {
     // do nothing
     if (*primesLength >= primeTarget + 1)
-        return;
+        return primes;
 
     int lastValue = *(primes + (*primesLength - 1));
     while (primeTarget + 1 != *primesLength)
@@ -30,11 +37,17 @@
         lastValue++;
         if (isPrime(lastValue))
         {
+            int *reallocated = realloc(primes, (*primesLength + 1) * sizeof(int));
+            if (!reallocated) { // test a result of realloc()
+                warn("realloc");
+                break;
+            }
+            primes = reallocated;
+            *(primes + *primesLength) = lastValue; // forgotten assignment
             (*primesLength)++;
-            primes = realloc(primes, *primesLength * sizeof(int));
-            *(primes + (*primesLength - 1));
         }
     }
+    return primes;
 }
 
 //* Checks if a given integer is prime